[PATCH v6 00/12] platform/x86: introduce p2sb_bar() helper

[PATCH v6 00/12] platform/x86: introduce p2sb_bar() helper

[Andy Shevchenko]

There are a few users that would like to utilize P2SB mechanism of hiding
and unhiding a device from the PCI configuration space.

Here is the series to consolidate p2sb handling code for existing users
and to provide a generic way for new comer(s).

It also includes a patch to enable GPIO controllers on Apollo Lake
when it's used with ABL bootloader w/o ACPI support.

The patch that brings the helper ("platform/x86/intel: Add Primary to
Sideband (P2SB) bridge support") has a commit message that sheds a light
on what the P2SB is and why this is needed.

I have tested this on Apollo Lake platform (I'm able to see SPI NOR and
since we have an ACPI device for GPIO I do not see any attempts to recreate
one).

The series is ready to be merged via MFD tree, but see below.

The series also includes updates for Simatic IPC drivers that partially
tagged by respective maintainers (the main question is if Pavel is okay
with the last three patches, since I believe Hans is okay with removing
some code under PDx86). Hence the first 8 patches can be merged right
away and the rest when Pavel does his review.

Changes in v6:
- added tag to patch 5 (Lee)
- incorporated Henning's series on top of v5

Changes in v5:
- rewritten patch 1 to use pci_scan_single_device() (Lukas, Bjorn)
- rebased patch 2 on top of the new Intel SPI NOR codebase
- fixed a potential bug and rewritten resource filling in patch 5 (Lee)
- added many different tags in a few patches (Jean, Wolfram, Henning)

Changes in v4:
- added tag to the entire series (Hans)
- added tag to pin control patch (Mika)
- dropped PCI core changes (PCI core doesn't want modifications to be made)
- as a consequence of the above merged necessary bits into p2sb.c
- added a check that p2sb is really hidden (Hans)
- added EDAC patches (reviewed by maintainer internally)

Changes in v3:
- resent with cover letter

Changes in v2:
- added parentheses around bus in macros (Joe)
- added tag (Jean)
- fixed indentation and wrapping in the header (Christoph)
- moved out of PCI realm to PDx86 as the best common denominator (Bjorn)
- added a verbose commit message to explain P2SB thingy (Bjorn)
- converted first parameter from pci_dev to pci_bus
- made first two parameters (bus and devfn) optional (Henning, Lee)
- added Intel pin control patch to the series (Henning, Mika)
- fixed English style in the commit message of one of MFD patch (Lee)
- added tags to my MFD LPC ICH patches (Lee)
- used consistently (c) (Lee)
- made indexing for MFD cell and resource arrays (Lee)
- fixed the resource size in i801 (Jean)

Andy Shevchenko (6):
  pinctrl: intel: Check against matching data instead of ACPI companion
  mfd: lpc_ich: Factor out lpc_ich_enable_spi_write()
  mfd: lpc_ich: Switch to generic p2sb_bar()
  i2c: i801: convert to use common P2SB accessor
  EDAC, pnd2: Use proper I/O accessors and address space annotation
  EDAC, pnd2: convert to use common P2SB accessor

Henning Schild (4):
  watchdog: simatic-ipc-wdt: convert to use P2SB accessor
  leds: simatic-ipc-leds: convert to use P2SB accessor
  platform/x86: simatic-ipc: drop custom P2SB bar code
  leds: simatic-ipc-leds-gpio: add GPIO version of Siemens driver

Jonathan Yong (1):
  platform/x86/intel: Add Primary to Sideband (P2SB) bridge support

Tan Jui Nee (1):
  mfd: lpc_ich: Add support for pinctrl in non-ACPI system

 drivers/edac/Kconfig                          |   1 +
 drivers/edac/pnd2_edac.c                      |  62 +++----
 drivers/i2c/busses/Kconfig                    |   1 +
 drivers/i2c/busses/i2c-i801.c                 |  39 +----
 drivers/leds/simple/Kconfig                   |   6 +-
 drivers/leds/simple/Makefile                  |   1 +
 drivers/leds/simple/simatic-ipc-leds-gpio.c   | 105 ++++++++++++
 drivers/leds/simple/simatic-ipc-leds.c        |  80 +--------
 drivers/mfd/Kconfig                           |   1 +
 drivers/mfd/lpc_ich.c                         | 161 ++++++++++++++----
 drivers/pinctrl/intel/pinctrl-intel.c         |  14 +-
 drivers/platform/x86/intel/Kconfig            |  12 ++
 drivers/platform/x86/intel/Makefile           |   2 +
 drivers/platform/x86/intel/p2sb.c             | 133 +++++++++++++++
 drivers/platform/x86/simatic-ipc.c            |  43 +----
 drivers/watchdog/Kconfig                      |   1 +
 drivers/watchdog/simatic-ipc-wdt.c            |  15 +-
 include/linux/platform_data/x86/p2sb.h        |  28 +++
 .../platform_data/x86/simatic-ipc-base.h      |   2 -
 19 files changed, 464 insertions(+), 243 deletions(-)
 create mode 100644 drivers/leds/simple/simatic-ipc-leds-gpio.c
 create mode 100644 drivers/platform/x86/intel/p2sb.c
 create mode 100644 include/linux/platform_data/x86/p2sb.h


base-commit: 40b58e42584bf5bd9230481dc8946f714fb387de
-- 
2.35.1

[PATCH v6 01/12] platform/x86/intel: Add Primary to Sideband (P2SB) bridge support

[Andy Shevchenko]

From: Jonathan Yong <jonathan.yong@intel.com>

SoC features such as GPIO are accessed via a reserved MMIO area,
we don't know its address but can obtain it from the BAR of
the P2SB device, that device is normally hidden so we have to
temporarily unhide it, read address and hide it back.

There are already a few users and at least one more is coming which
require an access to Primary to Sideband (P2SB) bridge in order
to get IO or MMIO BAR hidden by BIOS.

Create a library to access P2SB for x86 devices in a unified way.

Background information
======================
Note, the term "bridge" is used in the documentation and it has nothing
to do with a PCI (host) bridge as per the PCI specifications.

The P2SB is an interesting device by its nature and hardware design.
First of all, it has several devices in the hardware behind it. These
devices may or may not be represented as ACPI devices by a firmware.

It also has a hardwired (to 0s) the least significant bits of the
base address register which is represented by the only 64-bit BAR0.
It means that OS mustn't reallocate the BAR.

On top of that in some cases P2SB is represented by function 0 on PCI
slot (in terms of B:D.F) and according to the PCI specification any
other function can't be seen until function 0 is present and visible.

In the PCI configuration space of P2SB device the full 32-bit register
is allocated for the only purpose of hiding the entire P2SB device. As
per [3]:

  3.1.39 P2SB Control (P2SBC)—Offset E0h

  Hide Device (HIDE): When this bit is set, the P2SB will return 1s on
  any PCI Configuration Read on IOSF-P. All other transactions including
  PCI Configuration Writes on IOSF-P are unaffected by this. This does
  not affect reads performed on the IOSF-SB interface.

This doesn't prevent MMIO accesses, although preventing the OS from
assigning these addresses. The firmware on the affected platforms marks
the region as unusable (by cutting it off from the PCI host bridge
resources) as depicted in the Apollo Lake example below:

  PCI host bridge to bus 0000:00
  pci_bus 0000:00: root bus resource [io  0x0070-0x0077]
  pci_bus 0000:00: root bus resource [io  0x0000-0x006f window]
  pci_bus 0000:00: root bus resource [io  0x0078-0x0cf7 window]
  pci_bus 0000:00: root bus resource [io  0x0d00-0xffff window]
  pci_bus 0000:00: root bus resource [mem 0x7c000001-0x7fffffff window]
  pci_bus 0000:00: root bus resource [mem 0x7b800001-0x7bffffff window]
  pci_bus 0000:00: root bus resource [mem 0x80000000-0xcfffffff window]
  pci_bus 0000:00: root bus resource [mem 0xe0000000-0xefffffff window]
  pci_bus 0000:00: root bus resource [bus 00-ff]

The P2SB 16MB BAR is located at 0xd0000000-0xd0ffffff memory window.

The generic solution
====================
The generic solution for all cases when we need to access to the information
behind P2SB device is a library code where users ask for necessary resources
by demand and hence those users take care of not being run on the systems
where this access is not required.

The library provides the p2sb_bar() API to retrieve the MMIO of the BAR0 of
the device from P2SB device slot.

P2SB unconditional unhiding awareness
=====================================
Technically it's possible to unhide the P2SB device and devices on
the same PCI slot and access them at any time as needed. But there are
several potential issues with that:

 - the systems were never tested against such configuration and hence
   nobody knows what kind of bugs it may bring, especially when we talk
   about SPI NOR case which contains Intel FirmWare Image (IFWI) code
   (including BIOS) and already known to be problematic in the past for
   end users

 - the PCI by its nature is a hotpluggable bus and in case somebody
   attaches a driver to the functions of a P2SB slot device(s) the
   end user experience and system behaviour can be unpredictable

 - the kernel code would need some ugly hacks (or code looking as an
   ugly hack) under arch/x86/pci in order to enable these devices on
   only selected platforms (which may include CPU ID table followed by
   a potentially growing number of DMI strings

The future improvements
=======================
The future improvements with this code may go in order to gain some kind
of cache, if it's possible at all, to prevent unhiding and hiding many
times to take static information that may be saved once per boot.

Links
=====
[1]: https://lab.whitequark.org/notes/2017-11-08/accessing-intel-ich-pch-gpios/
[2]: https://cdrdv2.intel.com/v1/dl/getContent/332690?wapkw=332690
[3]: https://cdrdv2.intel.com/v1/dl/getContent/332691?wapkw=332691
[4]: https://medium.com/@jacksonchen_43335/bios-gpio-p2sb-70e9b829b403

Signed-off-by: Jonathan Yong <jonathan.yong@intel.com>
Co-developed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Tested-by: Henning Schild <henning.schild@siemens.com>
Acked-by: Hans de Goede <hdegoede@redhat.com>
Acked-by: Linus Walleij <linus.walleij@linaro.org>
---
 drivers/platform/x86/intel/Kconfig     |  12 +++
 drivers/platform/x86/intel/Makefile    |   2 +
 drivers/platform/x86/intel/p2sb.c      | 127 +++++++++++++++++++++++++
 include/linux/platform_data/x86/p2sb.h |  28 ++++++
 4 files changed, 169 insertions(+)
 create mode 100644 drivers/platform/x86/intel/p2sb.c
 create mode 100644 include/linux/platform_data/x86/p2sb.h

diff --git a/drivers/platform/x86/intel/Kconfig b/drivers/platform/x86/intel/Kconfig
index 794968bda115..c9cfbaae436b 100644
--- a/drivers/platform/x86/intel/Kconfig
+++ b/drivers/platform/x86/intel/Kconfig
@@ -70,6 +70,18 @@ config INTEL_OAKTRAIL
 	  enable/disable the Camera, WiFi, BT etc. devices. If in doubt, say Y
 	  here; it will only load on supported platforms.
 
+config P2SB
+	bool "Primary to Sideband (P2SB) bridge access support"
+	depends on PCI
+	help
+	  The Primary to Sideband (P2SB) bridge is an interface to some
+	  PCI devices connected through it. In particular, SPI NOR controller
+	  in Intel Apollo Lake SoC is one of such devices.
+
+	  The main purpose of this library is to unhide P2SB device in case
+	  firmware kept it hidden on some platforms in order to access devices
+	  behind it.
+
 config INTEL_BXTWC_PMIC_TMU
 	tristate "Intel Broxton Whiskey Cove TMU Driver"
 	depends on INTEL_SOC_PMIC_BXTWC
diff --git a/drivers/platform/x86/intel/Makefile b/drivers/platform/x86/intel/Makefile
index 717933dd0cfd..741a9404db98 100644
--- a/drivers/platform/x86/intel/Makefile
+++ b/drivers/platform/x86/intel/Makefile
@@ -28,6 +28,8 @@ intel_int0002_vgpio-y			:= int0002_vgpio.o
 obj-$(CONFIG_INTEL_INT0002_VGPIO)	+= intel_int0002_vgpio.o
 intel_oaktrail-y			:= oaktrail.o
 obj-$(CONFIG_INTEL_OAKTRAIL)		+= intel_oaktrail.o
+intel_p2sb-y				:= p2sb.o
+obj-$(CONFIG_P2SB)			+= intel_p2sb.o
 intel_sdsi-y				:= sdsi.o
 obj-$(CONFIG_INTEL_SDSI)		+= intel_sdsi.o
 intel_vsec-y				:= vsec.o
diff --git a/drivers/platform/x86/intel/p2sb.c b/drivers/platform/x86/intel/p2sb.c
new file mode 100644
index 000000000000..b598ef14dbc6
--- /dev/null
+++ b/drivers/platform/x86/intel/p2sb.c
@@ -0,0 +1,127 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Primary to Sideband (P2SB) bridge access support
+ *
+ * Copyright (c) 2017, 2021-2022 Intel Corporation.
+ *
+ * Authors: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
+ *	    Jonathan Yong <jonathan.yong@intel.com>
+ */
+
+#include <linux/bits.h>
+#include <linux/export.h>
+#include <linux/pci.h>
+#include <linux/platform_data/x86/p2sb.h>
+
+#include <asm/cpu_device_id.h>
+#include <asm/intel-family.h>
+
+#define P2SBC			0xe0
+#define P2SBC_HIDE		BIT(8)
+
+static const struct x86_cpu_id p2sb_cpu_ids[] = {
+	X86_MATCH_INTEL_FAM6_MODEL(ATOM_GOLDMONT,	PCI_DEVFN(13, 0)),
+	{}
+};
+
+static int p2sb_get_devfn(unsigned int *devfn)
+{
+	const struct x86_cpu_id *id;
+
+	id = x86_match_cpu(p2sb_cpu_ids);
+	if (!id)
+		return -ENODEV;
+
+	*devfn = (unsigned int)id->driver_data;
+	return 0;
+}
+
+static int p2sb_read_bar0(struct pci_dev *pdev, struct resource *mem)
+{
+	/* Copy resource from the first BAR of the device in question */
+	*mem = pdev->resource[0];
+	return 0;
+}
+
+static int p2sb_scan_and_read(struct pci_bus *bus, unsigned int devfn, struct resource *mem)
+{
+	struct pci_dev *pdev;
+	int ret;
+
+	pdev = pci_scan_single_device(bus, devfn);
+	if (!pdev)
+		return -ENODEV;
+
+	ret = p2sb_read_bar0(pdev, mem);
+
+	pci_stop_and_remove_bus_device(pdev);
+	return ret;
+}
+
+/**
+ * p2sb_bar - Get Primary to Sideband (P2SB) bridge device BAR
+ * @bus: PCI bus to communicate with
+ * @devfn: PCI slot and function to communicate with
+ * @mem: memory resource to be filled in
+ *
+ * The BIOS prevents the P2SB device from being enumerated by the PCI
+ * subsystem, so we need to unhide and hide it back to lookup the BAR.
+ *
+ * if @bus is NULL, the bus 0 in domain 0 will be used.
+ * If @devfn is 0, it will be replaced by devfn of the P2SB device.
+ *
+ * Caller must provide a valid pointer to @mem.
+ *
+ * Locking is handled by pci_rescan_remove_lock mutex.
+ *
+ * Return:
+ * 0 on success or appropriate errno value on error.
+ */
+int p2sb_bar(struct pci_bus *bus, unsigned int devfn, struct resource *mem)
+{
+	struct pci_dev *pdev_p2sb;
+	unsigned int devfn_p2sb;
+	u32 value = P2SBC_HIDE;
+	int ret;
+
+	/* Get devfn for P2SB device itself */
+	ret = p2sb_get_devfn(&devfn_p2sb);
+	if (ret)
+		return ret;
+
+	/* if @bus is NULL, use bus 0 in domain 0 */
+	bus = bus ?: pci_find_bus(0, 0);
+
+	/*
+	 * Prevent concurrent PCI bus scan from seeing the P2SB device and
+	 * removing via sysfs while it is temporarily exposed.
+	 */
+	pci_lock_rescan_remove();
+
+	/* Unhide the P2SB device, if needed */
+	pci_bus_read_config_dword(bus, devfn_p2sb, P2SBC, &value);
+	if (value & P2SBC_HIDE)
+		pci_bus_write_config_dword(bus, devfn_p2sb, P2SBC, 0);
+
+	pdev_p2sb = pci_scan_single_device(bus, devfn_p2sb);
+	if (devfn)
+		ret = p2sb_scan_and_read(bus, devfn, mem);
+	else
+		ret = p2sb_read_bar0(pdev_p2sb, mem);
+	pci_stop_and_remove_bus_device(pdev_p2sb);
+
+	/* Hide the P2SB device, if it was hidden */
+	if (value & P2SBC_HIDE)
+		pci_bus_write_config_dword(bus, devfn_p2sb, P2SBC, P2SBC_HIDE);
+
+	pci_unlock_rescan_remove();
+
+	if (ret)
+		return ret;
+
+	if (mem->flags == 0)
+		return -ENODEV;
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(p2sb_bar);
diff --git a/include/linux/platform_data/x86/p2sb.h b/include/linux/platform_data/x86/p2sb.h
new file mode 100644
index 000000000000..a1d5fddc8f13
--- /dev/null
+++ b/include/linux/platform_data/x86/p2sb.h
@@ -0,0 +1,28 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Primary to Sideband (P2SB) bridge access support
+ */
+
+#ifndef _PLATFORM_DATA_X86_P2SB_H
+#define _PLATFORM_DATA_X86_P2SB_H
+
+#include <linux/errno.h>
+#include <linux/kconfig.h>
+
+struct pci_bus;
+struct resource;
+
+#if IS_BUILTIN(CONFIG_P2SB)
+
+int p2sb_bar(struct pci_bus *bus, unsigned int devfn, struct resource *mem);
+
+#else /* CONFIG_P2SB */
+
+static inline int p2sb_bar(struct pci_bus *bus, unsigned int devfn, struct resource *mem)
+{
+	return -ENODEV;
+}
+
+#endif /* CONFIG_P2SB is not set */
+
+#endif /* _PLATFORM_DATA_X86_P2SB_H */
-- 
2.35.1

[PATCH v6 02/12] pinctrl: intel: Check against matching data instead of ACPI companion

[Andy Shevchenko]

In some cases we may get a platform device that has ACPI companion
which is different to the pin control described in the ACPI tables.
This is primarily happens when device is instantiated by board file.

In order to allow this device being enumerated, refactor
intel_pinctrl_get_soc_data() to check the matching data instead of
ACPI companion.

Reported-by: Henning Schild <henning.schild@siemens.com>
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Tested-by: Henning Schild <henning.schild@siemens.com>
Acked-by: Hans de Goede <hdegoede@redhat.com>
Acked-by: Mika Westerberg <mika.westerberg@linux.intel.com>
Acked-by: Linus Walleij <linus.walleij@linaro.org>
---
 drivers/pinctrl/intel/pinctrl-intel.c | 14 ++++++--------
 1 file changed, 6 insertions(+), 8 deletions(-)

diff --git a/drivers/pinctrl/intel/pinctrl-intel.c b/drivers/pinctrl/intel/pinctrl-intel.c
index ffc045f7bf00..fd093e36c3a8 100644
--- a/drivers/pinctrl/intel/pinctrl-intel.c
+++ b/drivers/pinctrl/intel/pinctrl-intel.c
@@ -1641,16 +1641,14 @@ EXPORT_SYMBOL_GPL(intel_pinctrl_probe_by_uid);
 
 const struct intel_pinctrl_soc_data *intel_pinctrl_get_soc_data(struct platform_device *pdev)
 {
+	const struct intel_pinctrl_soc_data * const *table;
 	const struct intel_pinctrl_soc_data *data = NULL;
-	const struct intel_pinctrl_soc_data **table;
-	struct acpi_device *adev;
-	unsigned int i;
 
-	adev = ACPI_COMPANION(&pdev->dev);
-	if (adev) {
-		const void *match = device_get_match_data(&pdev->dev);
+	table = device_get_match_data(&pdev->dev);
+	if (table) {
+		struct acpi_device *adev = ACPI_COMPANION(&pdev->dev);
+		unsigned int i;
 
-		table = (const struct intel_pinctrl_soc_data **)match;
 		for (i = 0; table[i]; i++) {
 			if (!strcmp(adev->pnp.unique_id, table[i]->uid)) {
 				data = table[i];
@@ -1664,7 +1662,7 @@ const struct intel_pinctrl_soc_data *intel_pinctrl_get_soc_data(struct platform_
 		if (!id)
 			return ERR_PTR(-ENODEV);
 
-		table = (const struct intel_pinctrl_soc_data **)id->driver_data;
+		table = (const struct intel_pinctrl_soc_data * const *)id->driver_data;
 		data = table[pdev->id];
 	}
 
-- 
2.35.1

[PATCH v6 03/12] mfd: lpc_ich: Factor out lpc_ich_enable_spi_write()

[Andy Shevchenko]

Factor out duplicate code to lpc_ich_enable_spi_write() helper function.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Tested-by: Henning Schild <henning.schild@siemens.com>
Acked-by: Hans de Goede <hdegoede@redhat.com>
Acked-by: Linus Walleij <linus.walleij@linaro.org>
Acked-for-MFD-by: Lee Jones <lee.jones@linaro.org>
---
 drivers/mfd/lpc_ich.c | 31 ++++++++++++++-----------------
 1 file changed, 14 insertions(+), 17 deletions(-)

diff --git a/drivers/mfd/lpc_ich.c b/drivers/mfd/lpc_ich.c
index 9ffab9aafd81..d9175cb8a2d5 100644
--- a/drivers/mfd/lpc_ich.c
+++ b/drivers/mfd/lpc_ich.c
@@ -1100,35 +1100,32 @@ static bool lpc_ich_byt_set_writeable(void __iomem *base, void *data)
 	return val & BYT_BCR_WPD;
 }
 
-static bool lpc_ich_lpt_set_writeable(void __iomem *base, void *data)
+static bool lpc_ich_set_writeable(struct pci_bus *bus, unsigned int devfn)
 {
-	struct pci_dev *pdev = data;
 	u32 bcr;
 
-	pci_read_config_dword(pdev, BCR, &bcr);
+	pci_bus_read_config_dword(bus, devfn, BCR, &bcr);
 	if (!(bcr & BCR_WPD)) {
 		bcr |= BCR_WPD;
-		pci_write_config_dword(pdev, BCR, bcr);
-		pci_read_config_dword(pdev, BCR, &bcr);
+		pci_bus_write_config_dword(bus, devfn, BCR, bcr);
+		pci_bus_read_config_dword(bus, devfn, BCR, &bcr);
 	}
 
 	return bcr & BCR_WPD;
 }
 
-static bool lpc_ich_bxt_set_writeable(void __iomem *base, void *data)
+static bool lpc_ich_lpt_set_writeable(void __iomem *base, void *data)
 {
-	unsigned int spi = PCI_DEVFN(13, 2);
-	struct pci_bus *bus = data;
-	u32 bcr;
+	struct pci_dev *pdev = data;
 
-	pci_bus_read_config_dword(bus, spi, BCR, &bcr);
-	if (!(bcr & BCR_WPD)) {
-		bcr |= BCR_WPD;
-		pci_bus_write_config_dword(bus, spi, BCR, bcr);
-		pci_bus_read_config_dword(bus, spi, BCR, &bcr);
-	}
+	return lpc_ich_set_writeable(pdev->bus, pdev->devfn);
+}
 
-	return bcr & BCR_WPD;
+static bool lpc_ich_bxt_set_writeable(void __iomem *base, void *data)
+{
+	struct pci_dev *pdev = data;
+
+	return lpc_ich_set_writeable(pdev->bus, PCI_DEVFN(13, 2));
 }
 
 static int lpc_ich_init_spi(struct pci_dev *dev)
@@ -1185,7 +1182,7 @@ static int lpc_ich_init_spi(struct pci_dev *dev)
 			res->end = res->start + SPIBASE_APL_SZ - 1;
 
 			info->set_writeable = lpc_ich_bxt_set_writeable;
-			info->data = bus;
+			info->data = dev;
 		}
 
 		pci_bus_write_config_byte(bus, p2sb, 0xe1, 0x1);
-- 
2.35.1

[PATCH v6 04/12] mfd: lpc_ich: Switch to generic p2sb_bar()

[Andy Shevchenko]

Instead of open coding p2sb_bar() functionality we are going to
use generic library. There is one more user en route.

This is more than just a clean-up. It also fixes a potential issue
seen when SPI BAR is 64-bit. The current code works if and only if
the PCI BAR of the hidden device is inside 4G address space. In case
when firmware decides to go above 4G, we will get a wrong address.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Tested-by: Henning Schild <henning.schild@siemens.com>
Acked-by: Hans de Goede <hdegoede@redhat.com>
Acked-by: Linus Walleij <linus.walleij@linaro.org>
Acked-for-MFD-by: Lee Jones <lee.jones@linaro.org>
---
 drivers/mfd/Kconfig   |  1 +
 drivers/mfd/lpc_ich.c | 27 ++++++++-------------------
 2 files changed, 9 insertions(+), 19 deletions(-)

diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
index 3b59456f5545..9566341de470 100644
--- a/drivers/mfd/Kconfig
+++ b/drivers/mfd/Kconfig
@@ -572,6 +572,7 @@ config LPC_ICH
 	tristate "Intel ICH LPC"
 	depends on PCI
 	select MFD_CORE
+	select P2SB if X86
 	help
 	  The LPC bridge function of the Intel ICH provides support for
 	  many functional units. This driver provides needed support for
diff --git a/drivers/mfd/lpc_ich.c b/drivers/mfd/lpc_ich.c
index d9175cb8a2d5..e360651c5406 100644
--- a/drivers/mfd/lpc_ich.c
+++ b/drivers/mfd/lpc_ich.c
@@ -45,6 +45,7 @@
 #include <linux/mfd/core.h>
 #include <linux/mfd/lpc_ich.h>
 #include <linux/platform_data/itco_wdt.h>
+#include <linux/platform_data/x86/p2sb.h>
 
 #define ACPIBASE		0x40
 #define ACPIBASE_GPE_OFF	0x28
@@ -71,8 +72,6 @@
 #define BCR			0xdc
 #define BCR_WPD			BIT(0)
 
-#define SPIBASE_APL_SZ		4096
-
 #define GPIOBASE_ICH0		0x58
 #define GPIOCTRL_ICH0		0x5C
 #define GPIOBASE_ICH6		0x48
@@ -1134,6 +1133,7 @@ static int lpc_ich_init_spi(struct pci_dev *dev)
 	struct resource *res = &intel_spi_res[0];
 	struct intel_spi_boardinfo *info;
 	u32 spi_base, rcba;
+	int ret;
 
 	info = devm_kzalloc(&dev->dev, sizeof(*info), GFP_KERNEL);
 	if (!info)
@@ -1164,30 +1164,19 @@ static int lpc_ich_init_spi(struct pci_dev *dev)
 		}
 		break;
 
-	case INTEL_SPI_BXT: {
-		unsigned int p2sb = PCI_DEVFN(13, 0);
-		unsigned int spi = PCI_DEVFN(13, 2);
-		struct pci_bus *bus = dev->bus;
-
+	case INTEL_SPI_BXT:
 		/*
 		 * The P2SB is hidden by BIOS and we need to unhide it in
 		 * order to read BAR of the SPI flash device. Once that is
 		 * done we hide it again.
 		 */
-		pci_bus_write_config_byte(bus, p2sb, 0xe1, 0x0);
-		pci_bus_read_config_dword(bus, spi, PCI_BASE_ADDRESS_0,
-					  &spi_base);
-		if (spi_base != ~0) {
-			res->start = spi_base & 0xfffffff0;
-			res->end = res->start + SPIBASE_APL_SZ - 1;
-
-			info->set_writeable = lpc_ich_bxt_set_writeable;
-			info->data = dev;
-		}
+		ret = p2sb_bar(dev->bus, PCI_DEVFN(13, 2), res);
+		if (ret)
+			return ret;
 
-		pci_bus_write_config_byte(bus, p2sb, 0xe1, 0x1);
+		info->set_writeable = lpc_ich_bxt_set_writeable;
+		info->data = dev;
 		break;
-	}
 
 	default:
 		return -EINVAL;
-- 
2.35.1

[PATCH v6 05/12] mfd: lpc_ich: Add support for pinctrl in non-ACPI system

[Andy Shevchenko]

From: Tan Jui Nee <jui.nee.tan@intel.com>

Add support for non-ACPI systems, such as system that uses
Advanced Boot Loader (ABL) whereby a platform device has to be created
in order to bind with pin control and GPIO.

At the moment, Intel Apollo Lake In-Vehicle Infotainment (IVI) system
requires a driver to hide and unhide P2SB to lookup P2SB BAR and pass
the PCI BAR address to GPIO.

Signed-off-by: Tan Jui Nee <jui.nee.tan@intel.com>
Co-developed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Tested-by: Henning Schild <henning.schild@siemens.com>
Acked-by: Hans de Goede <hdegoede@redhat.com>
Acked-by: Linus Walleij <linus.walleij@linaro.org>
Acked-for-MFD-by: Lee Jones <lee.jones@linaro.org>
---
 drivers/mfd/lpc_ich.c | 105 +++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 104 insertions(+), 1 deletion(-)

diff --git a/drivers/mfd/lpc_ich.c b/drivers/mfd/lpc_ich.c
index e360651c5406..650951f89f1c 100644
--- a/drivers/mfd/lpc_ich.c
+++ b/drivers/mfd/lpc_ich.c
@@ -8,7 +8,8 @@
  *  Configuration Registers.
  *
  *  This driver is derived from lpc_sch.
-
+ *
+ *  Copyright (c) 2017, 2021-2022 Intel Corporation
  *  Copyright (c) 2011 Extreme Engineering Solution, Inc.
  *  Author: Aaron Sierra <asierra@xes-inc.com>
  *
@@ -42,6 +43,7 @@
 #include <linux/errno.h>
 #include <linux/acpi.h>
 #include <linux/pci.h>
+#include <linux/pinctrl/pinctrl.h>
 #include <linux/mfd/core.h>
 #include <linux/mfd/lpc_ich.h>
 #include <linux/platform_data/itco_wdt.h>
@@ -142,6 +144,73 @@ static struct mfd_cell lpc_ich_gpio_cell = {
 	.ignore_resource_conflicts = true,
 };
 
+#define APL_GPIO_NORTH		0
+#define APL_GPIO_NORTHWEST	1
+#define APL_GPIO_WEST		2
+#define APL_GPIO_SOUTHWEST	3
+#define APL_GPIO_NR_DEVICES	4
+
+/* Offset data for Apollo Lake GPIO controllers */
+static resource_size_t apl_gpio_offsets[APL_GPIO_NR_DEVICES] = {
+	[APL_GPIO_NORTH]	= 0xc50000,
+	[APL_GPIO_NORTHWEST]	= 0xc40000,
+	[APL_GPIO_WEST]		= 0xc70000,
+	[APL_GPIO_SOUTHWEST]	= 0xc00000,
+};
+
+#define APL_GPIO_RESOURCE_SIZE		0x1000
+
+#define APL_GPIO_IRQ			14
+
+static struct resource apl_gpio_resources[APL_GPIO_NR_DEVICES][2] = {
+	[APL_GPIO_NORTH] = {
+		DEFINE_RES_MEM(0, 0),
+		DEFINE_RES_IRQ(APL_GPIO_IRQ),
+	},
+	[APL_GPIO_NORTHWEST] = {
+		DEFINE_RES_MEM(0, 0),
+		DEFINE_RES_IRQ(APL_GPIO_IRQ),
+	},
+	[APL_GPIO_WEST] = {
+		DEFINE_RES_MEM(0, 0),
+		DEFINE_RES_IRQ(APL_GPIO_IRQ),
+	},
+	[APL_GPIO_SOUTHWEST] = {
+		DEFINE_RES_MEM(0, 0),
+		DEFINE_RES_IRQ(APL_GPIO_IRQ),
+	},
+};
+
+static const struct mfd_cell apl_gpio_devices[APL_GPIO_NR_DEVICES] = {
+	[APL_GPIO_NORTH] = {
+		.name = "apollolake-pinctrl",
+		.id = APL_GPIO_NORTH,
+		.num_resources = ARRAY_SIZE(apl_gpio_resources[APL_GPIO_NORTH]),
+		.resources = apl_gpio_resources[APL_GPIO_NORTH],
+		.ignore_resource_conflicts = true,
+	},
+	[APL_GPIO_NORTHWEST] = {
+		.name = "apollolake-pinctrl",
+		.id = APL_GPIO_NORTHWEST,
+		.num_resources = ARRAY_SIZE(apl_gpio_resources[APL_GPIO_NORTHWEST]),
+		.resources = apl_gpio_resources[APL_GPIO_NORTHWEST],
+		.ignore_resource_conflicts = true,
+	},
+	[APL_GPIO_WEST] = {
+		.name = "apollolake-pinctrl",
+		.id = APL_GPIO_WEST,
+		.num_resources = ARRAY_SIZE(apl_gpio_resources[APL_GPIO_WEST]),
+		.resources = apl_gpio_resources[APL_GPIO_WEST],
+		.ignore_resource_conflicts = true,
+	},
+	[APL_GPIO_SOUTHWEST] = {
+		.name = "apollolake-pinctrl",
+		.id = APL_GPIO_SOUTHWEST,
+		.num_resources = ARRAY_SIZE(apl_gpio_resources[APL_GPIO_SOUTHWEST]),
+		.resources = apl_gpio_resources[APL_GPIO_SOUTHWEST],
+		.ignore_resource_conflicts = true,
+	},
+};
 
 static struct mfd_cell lpc_ich_spi_cell = {
 	.name = "intel-spi",
@@ -1085,6 +1154,34 @@ static int lpc_ich_init_wdt(struct pci_dev *dev)
 	return ret;
 }
 
+static int lpc_ich_init_pinctrl(struct pci_dev *dev)
+{
+	struct resource base;
+	unsigned int i;
+	int ret;
+
+	/* Check, if GPIO has been exported as an ACPI device */
+	if (acpi_dev_present("INT3452", NULL, -1))
+		return -EEXIST;
+
+	ret = p2sb_bar(dev->bus, 0, &base);
+	if (ret)
+		return ret;
+
+	for (i = 0; i < ARRAY_SIZE(apl_gpio_devices); i++) {
+		struct resource *mem = &apl_gpio_resources[i][0];
+		resource_size_t offset = apl_gpio_offsets[i];
+
+		/* Fill MEM resource */
+		mem->start = base.start + offset;
+		mem->end = base.start + offset + APL_GPIO_RESOURCE_SIZE - 1;
+		mem->flags = base.flags;
+	}
+
+	return mfd_add_devices(&dev->dev, 0, apl_gpio_devices,
+			       ARRAY_SIZE(apl_gpio_devices), NULL, 0, NULL);
+}
+
 static bool lpc_ich_byt_set_writeable(void __iomem *base, void *data)
 {
 	u32 val;
@@ -1235,6 +1332,12 @@ static int lpc_ich_probe(struct pci_dev *dev,
 			cell_added = true;
 	}
 
+	if (priv->chipset == LPC_APL) {
+		ret = lpc_ich_init_pinctrl(dev);
+		if (!ret)
+			cell_added = true;
+	}
+
 	if (lpc_chipset_info[priv->chipset].spi_type) {
 		ret = lpc_ich_init_spi(dev);
 		if (!ret)
-- 
2.35.1

[PATCH v6 06/12] i2c: i801: convert to use common P2SB accessor

[Andy Shevchenko]

Since we have a common P2SB accessor in tree we may use it instead of
open coded variants.

Replace custom code by p2sb_bar() call.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Tested-by: Henning Schild <henning.schild@siemens.com>
Acked-by: Hans de Goede <hdegoede@redhat.com>
Acked-by: Linus Walleij <linus.walleij@linaro.org>
Reviewed-by: Jean Delvare <jdelvare@suse.de>
Acked-by: Wolfram Sang <wsa@kernel.org>
---
 drivers/i2c/busses/Kconfig        |  1 +
 drivers/i2c/busses/i2c-i801.c     | 39 +++++++------------------------
 drivers/platform/x86/intel/p2sb.c |  6 +++++
 3 files changed, 16 insertions(+), 30 deletions(-)

diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
index a1bae59208e3..4d0a195ca3ef 100644
--- a/drivers/i2c/busses/Kconfig
+++ b/drivers/i2c/busses/Kconfig
@@ -108,6 +108,7 @@ config I2C_HIX5HD2
 config I2C_I801
 	tristate "Intel 82801 (ICH/PCH)"
 	depends on PCI
+	select P2SB if X86
 	select CHECK_SIGNATURE if X86 && DMI
 	select I2C_SMBUS
 	help
diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
index ff706349bdfb..f7a0bb372e8e 100644
--- a/drivers/i2c/busses/i2c-i801.c
+++ b/drivers/i2c/busses/i2c-i801.c
@@ -111,6 +111,7 @@
 #include <linux/err.h>
 #include <linux/platform_device.h>
 #include <linux/platform_data/itco_wdt.h>
+#include <linux/platform_data/x86/p2sb.h>
 #include <linux/pm_runtime.h>
 #include <linux/mutex.h>
 
@@ -140,7 +141,6 @@
 #define TCOBASE		0x050
 #define TCOCTL		0x054
 
-#define SBREG_BAR		0x10
 #define SBREG_SMBCTRL		0xc6000c
 #define SBREG_SMBCTRL_DNV	0xcf000c
 
@@ -1482,45 +1482,24 @@ i801_add_tco_spt(struct i801_priv *priv, struct pci_dev *pci_dev,
 		.version = 4,
 	};
 	struct resource *res;
-	unsigned int devfn;
-	u64 base64_addr;
-	u32 base_addr;
-	u8 hidden;
+	int ret;
 
 	/*
 	 * We must access the NO_REBOOT bit over the Primary to Sideband
-	 * bridge (P2SB). The BIOS prevents the P2SB device from being
-	 * enumerated by the PCI subsystem, so we need to unhide/hide it
-	 * to lookup the P2SB BAR.
+	 * (P2SB) bridge.
 	 */
-	pci_lock_rescan_remove();
-
-	devfn = PCI_DEVFN(PCI_SLOT(pci_dev->devfn), 1);
-
-	/* Unhide the P2SB device, if it is hidden */
-	pci_bus_read_config_byte(pci_dev->bus, devfn, 0xe1, &hidden);
-	if (hidden)
-		pci_bus_write_config_byte(pci_dev->bus, devfn, 0xe1, 0x0);
-
-	pci_bus_read_config_dword(pci_dev->bus, devfn, SBREG_BAR, &base_addr);
-	base64_addr = base_addr & 0xfffffff0;
-
-	pci_bus_read_config_dword(pci_dev->bus, devfn, SBREG_BAR + 0x4, &base_addr);
-	base64_addr |= (u64)base_addr << 32;
-
-	/* Hide the P2SB device, if it was hidden before */
-	if (hidden)
-		pci_bus_write_config_byte(pci_dev->bus, devfn, 0xe1, hidden);
-	pci_unlock_rescan_remove();
 
 	res = &tco_res[1];
+	ret = p2sb_bar(pci_dev->bus, 0, res);
+	if (ret)
+		return ERR_PTR(ret);
+
 	if (pci_dev->device == PCI_DEVICE_ID_INTEL_DNV_SMBUS)
-		res->start = (resource_size_t)base64_addr + SBREG_SMBCTRL_DNV;
+		res->start += SBREG_SMBCTRL_DNV;
 	else
-		res->start = (resource_size_t)base64_addr + SBREG_SMBCTRL;
+		res->start += SBREG_SMBCTRL;
 
 	res->end = res->start + 3;
-	res->flags = IORESOURCE_MEM;
 
 	return platform_device_register_resndata(&pci_dev->dev, "iTCO_wdt", -1,
 					tco_res, 2, &pldata, sizeof(pldata));
diff --git a/drivers/platform/x86/intel/p2sb.c b/drivers/platform/x86/intel/p2sb.c
index b598ef14dbc6..fb2e141f3eb8 100644
--- a/drivers/platform/x86/intel/p2sb.c
+++ b/drivers/platform/x86/intel/p2sb.c
@@ -21,6 +21,12 @@
 
 static const struct x86_cpu_id p2sb_cpu_ids[] = {
 	X86_MATCH_INTEL_FAM6_MODEL(ATOM_GOLDMONT,	PCI_DEVFN(13, 0)),
+	X86_MATCH_INTEL_FAM6_MODEL(ATOM_GOLDMONT_D,	PCI_DEVFN(31, 1)),
+	X86_MATCH_INTEL_FAM6_MODEL(ATOM_SILVERMONT_D,	PCI_DEVFN(31, 1)),
+	X86_MATCH_INTEL_FAM6_MODEL(KABYLAKE,		PCI_DEVFN(31, 1)),
+	X86_MATCH_INTEL_FAM6_MODEL(KABYLAKE_L,		PCI_DEVFN(31, 1)),
+	X86_MATCH_INTEL_FAM6_MODEL(SKYLAKE,		PCI_DEVFN(31, 1)),
+	X86_MATCH_INTEL_FAM6_MODEL(SKYLAKE_L,		PCI_DEVFN(31, 1)),
 	{}
 };
 
-- 
2.35.1

[PATCH v6 07/12] EDAC, pnd2: Use proper I/O accessors and address space annotation

[Andy Shevchenko]

The driver uses rather voodoo kind of castings and I/O accessors.
Replace it with proper __iomem annotation and readl()/readq() calls.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Tested-by: Henning Schild <henning.schild@siemens.com>
Reviewed-by: Tony Luck <tony.luck@intel.com>
---
 drivers/edac/pnd2_edac.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/edac/pnd2_edac.c b/drivers/edac/pnd2_edac.c
index c94ca1f790c4..7d1df120e24c 100644
--- a/drivers/edac/pnd2_edac.c
+++ b/drivers/edac/pnd2_edac.c
@@ -265,7 +265,7 @@ static u64 get_sideband_reg_base_addr(void)
 static int dnv_rd_reg(int port, int off, int op, void *data, size_t sz, char *name)
 {
 	struct pci_dev *pdev;
-	char *base;
+	void __iomem *base;
 	u64 addr;
 	unsigned long size;
 
@@ -297,8 +297,9 @@ static int dnv_rd_reg(int port, int off, int op, void *data, size_t sz, char *na
 			return -ENODEV;
 
 		if (sz == 8)
-			*(u32 *)(data + 4) = *(u32 *)(base + off + 4);
-		*(u32 *)data = *(u32 *)(base + off);
+			*(u64 *)data = readq(base + off);
+		else
+			*(u32 *)data = readl(base + off);
 
 		iounmap(base);
 	}
-- 
2.35.1

[PATCH v6 08/12] EDAC, pnd2: convert to use common P2SB accessor

[Andy Shevchenko]

Since we have a common P2SB accessor in tree we may use it instead of
open coded variants.

Replace custom code by p2sb_bar() call.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Tested-by: Henning Schild <henning.schild@siemens.com>
Reviewed-by: Tony Luck <tony.luck@intel.com>
---
 drivers/edac/Kconfig     |  1 +
 drivers/edac/pnd2_edac.c | 55 ++++++++++++----------------------------
 2 files changed, 17 insertions(+), 39 deletions(-)

diff --git a/drivers/edac/Kconfig b/drivers/edac/Kconfig
index d3e2477948c8..17562cf1fe97 100644
--- a/drivers/edac/Kconfig
+++ b/drivers/edac/Kconfig
@@ -263,6 +263,7 @@ config EDAC_I10NM
 config EDAC_PND2
 	tristate "Intel Pondicherry2"
 	depends on PCI && X86_64 && X86_MCE_INTEL
+	select P2SB if X86
 	help
 	  Support for error detection and correction on the Intel
 	  Pondicherry2 Integrated Memory Controller. This SoC IP is
diff --git a/drivers/edac/pnd2_edac.c b/drivers/edac/pnd2_edac.c
index 7d1df120e24c..a20b299f1202 100644
--- a/drivers/edac/pnd2_edac.c
+++ b/drivers/edac/pnd2_edac.c
@@ -28,6 +28,8 @@
 #include <linux/bitmap.h>
 #include <linux/math64.h>
 #include <linux/mod_devicetable.h>
+#include <linux/platform_data/x86/p2sb.h>
+
 #include <asm/cpu_device_id.h>
 #include <asm/intel-family.h>
 #include <asm/processor.h>
@@ -232,42 +234,14 @@ static u64 get_mem_ctrl_hub_base_addr(void)
 	return U64_LSHIFT(hi.base, 32) | U64_LSHIFT(lo.base, 15);
 }
 
-static u64 get_sideband_reg_base_addr(void)
-{
-	struct pci_dev *pdev;
-	u32 hi, lo;
-	u8 hidden;
-
-	pdev = pci_get_device(PCI_VENDOR_ID_INTEL, 0x19dd, NULL);
-	if (pdev) {
-		/* Unhide the P2SB device, if it's hidden */
-		pci_read_config_byte(pdev, 0xe1, &hidden);
-		if (hidden)
-			pci_write_config_byte(pdev, 0xe1, 0);
-
-		pci_read_config_dword(pdev, 0x10, &lo);
-		pci_read_config_dword(pdev, 0x14, &hi);
-		lo &= 0xfffffff0;
-
-		/* Hide the P2SB device, if it was hidden before */
-		if (hidden)
-			pci_write_config_byte(pdev, 0xe1, hidden);
-
-		pci_dev_put(pdev);
-		return (U64_LSHIFT(hi, 32) | U64_LSHIFT(lo, 0));
-	} else {
-		return 0xfd000000;
-	}
-}
-
 #define DNV_MCHBAR_SIZE  0x8000
 #define DNV_SB_PORT_SIZE 0x10000
 static int dnv_rd_reg(int port, int off, int op, void *data, size_t sz, char *name)
 {
 	struct pci_dev *pdev;
 	void __iomem *base;
-	u64 addr;
-	unsigned long size;
+	struct resource r;
+	int ret;
 
 	if (op == 4) {
 		pdev = pci_get_device(PCI_VENDOR_ID_INTEL, 0x1980, NULL);
@@ -279,20 +253,23 @@ static int dnv_rd_reg(int port, int off, int op, void *data, size_t sz, char *na
 	} else {
 		/* MMIO via memory controller hub base address */
 		if (op == 0 && port == 0x4c) {
-			addr = get_mem_ctrl_hub_base_addr();
-			if (!addr)
+			memset(&r, 0, sizeof(r));
+
+			r.start = get_mem_ctrl_hub_base_addr();
+			if (!r.start)
 				return -ENODEV;
-			size = DNV_MCHBAR_SIZE;
+			r.end = r.start + DNV_MCHBAR_SIZE - 1;
 		} else {
 			/* MMIO via sideband register base address */
-			addr = get_sideband_reg_base_addr();
-			if (!addr)
-				return -ENODEV;
-			addr += (port << 16);
-			size = DNV_SB_PORT_SIZE;
+			ret = p2sb_bar(NULL, 0, &r);
+			if (ret)
+				return ret;
+
+			r.start += (port << 16);
+			r.end = r.start + DNV_SB_PORT_SIZE - 1;
 		}
 
-		base = ioremap((resource_size_t)addr, size);
+		base = ioremap(r.start, resource_size(&r));
 		if (!base)
 			return -ENODEV;
 
-- 
2.35.1

[PATCH v6 09/12] watchdog: simatic-ipc-wdt: convert to use P2SB accessor

[Andy Shevchenko]

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

Since we have a common P2SB accessor in tree we may use it instead of
open coded variants.

Replace custom code by p2sb_bar() call.

Signed-off-by: Henning Schild <henning.schild@siemens.com>
Acked-by: Guenter Roeck <linux@roeck-us.net>
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/watchdog/Kconfig           |  1 +
 drivers/watchdog/simatic-ipc-wdt.c | 15 ++++++++-------
 2 files changed, 9 insertions(+), 7 deletions(-)

diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
index 32fd37698932..0796f6a9e8ff 100644
--- a/drivers/watchdog/Kconfig
+++ b/drivers/watchdog/Kconfig
@@ -1647,6 +1647,7 @@ config SIEMENS_SIMATIC_IPC_WDT
 	tristate "Siemens Simatic IPC Watchdog"
 	depends on SIEMENS_SIMATIC_IPC
 	select WATCHDOG_CORE
+	select P2SB
 	help
 	  This driver adds support for several watchdogs found in Industrial
 	  PCs from Siemens.
diff --git a/drivers/watchdog/simatic-ipc-wdt.c b/drivers/watchdog/simatic-ipc-wdt.c
index 8bac793c63fb..6599695dc672 100644
--- a/drivers/watchdog/simatic-ipc-wdt.c
+++ b/drivers/watchdog/simatic-ipc-wdt.c
@@ -16,6 +16,7 @@
 #include <linux/kernel.h>
 #include <linux/module.h>
 #include <linux/pci.h>
+#include <linux/platform_data/x86/p2sb.h>
 #include <linux/platform_data/x86/simatic-ipc-base.h>
 #include <linux/platform_device.h>
 #include <linux/sizes.h>
@@ -54,9 +55,9 @@ static struct resource io_resource_trigger =
 	DEFINE_RES_IO_NAMED(WD_TRIGGER_IOADR, SZ_1,
 			    KBUILD_MODNAME " WD_TRIGGER_IOADR");
 
-/* the actual start will be discovered with pci, 0 is a placeholder */
+/* the actual start will be discovered with p2sb, 0 is a placeholder */
 static struct resource mem_resource =
-	DEFINE_RES_MEM_NAMED(0, SZ_4, "WD_RESET_BASE_ADR");
+	DEFINE_RES_MEM_NAMED(0, 0, "WD_RESET_BASE_ADR");
 
 static u32 wd_timeout_table[] = {2, 4, 6, 8, 16, 32, 48, 64 };
 static void __iomem *wd_reset_base_addr;
@@ -150,6 +151,7 @@ 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;
+	int ret;
 
 	switch (plat->devmode) {
 	case SIMATIC_IPC_DEVICE_227E:
@@ -190,15 +192,14 @@ static int simatic_ipc_wdt_probe(struct platform_device *pdev)
 	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;
+		ret = p2sb_bar(NULL, 0, res);
+		if (ret)
+			return ret;
 
 		/* do the final address calculation */
 		res->start = res->start + (GPIO_COMMUNITY0_PORT_ID << 16) +
 			     PAD_CFG_DW0_GPP_A_23;
-		res->end += res->start;
+		res->end = res->start + SZ_4 - 1;
 
 		wd_reset_base_addr = devm_ioremap_resource(dev, res);
 		if (IS_ERR(wd_reset_base_addr))
-- 
2.35.1

[PATCH v6 10/12] leds: simatic-ipc-leds: convert to use P2SB accessor

[Andy Shevchenko]

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

Since we have a common P2SB accessor in tree we may use it instead of
open coded variants.

Replace custom code by p2sb_bar() call.

Signed-off-by: Henning Schild <henning.schild@siemens.com>
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/leds/simple/Kconfig            |  1 +
 drivers/leds/simple/simatic-ipc-leds.c | 14 +++++++-------
 2 files changed, 8 insertions(+), 7 deletions(-)

diff --git a/drivers/leds/simple/Kconfig b/drivers/leds/simple/Kconfig
index 9f6a68336659..bbf8cff3c3f6 100644
--- a/drivers/leds/simple/Kconfig
+++ b/drivers/leds/simple/Kconfig
@@ -3,6 +3,7 @@ config LEDS_SIEMENS_SIMATIC_IPC
 	tristate "LED driver for Siemens Simatic IPCs"
 	depends on LEDS_CLASS
 	depends on SIEMENS_SIMATIC_IPC
+	select P2SB
 	help
 	  This option enables support for the LEDs of several Industrial PCs
 	  from Siemens.
diff --git a/drivers/leds/simple/simatic-ipc-leds.c b/drivers/leds/simple/simatic-ipc-leds.c
index 078d43f5ba38..2e7597c143d8 100644
--- a/drivers/leds/simple/simatic-ipc-leds.c
+++ b/drivers/leds/simple/simatic-ipc-leds.c
@@ -15,6 +15,7 @@
 #include <linux/leds.h>
 #include <linux/module.h>
 #include <linux/pci.h>
+#include <linux/platform_data/x86/p2sb.h>
 #include <linux/platform_data/x86/simatic-ipc-base.h>
 #include <linux/platform_device.h>
 #include <linux/sizes.h>
@@ -38,8 +39,8 @@ static struct simatic_ipc_led simatic_ipc_leds_io[] = {
 	{ }
 };
 
-/* the actual start will be discovered with PCI, 0 is a placeholder */
-static struct resource simatic_ipc_led_mem_res = DEFINE_RES_MEM_NAMED(0, SZ_4K, KBUILD_MODNAME);
+/* the actual start will be discovered with p2sb, 0 is a placeholder */
+static struct resource simatic_ipc_led_mem_res = DEFINE_RES_MEM_NAMED(0, 0, KBUILD_MODNAME);
 
 static void __iomem *simatic_ipc_led_memory;
 
@@ -145,14 +146,13 @@ static int simatic_ipc_leds_probe(struct platform_device *pdev)
 		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;
+		err = p2sb_bar(NULL, 0, res);
+		if (err)
+			return err;
 
 		/* do the final address calculation */
 		res->start = res->start + (0xC5 << 16);
-		res->end += res->start;
+		res->end = res->start + SZ_4K - 1;
 
 		simatic_ipc_led_memory = devm_ioremap_resource(dev, res);
 		if (IS_ERR(simatic_ipc_led_memory))
-- 
2.35.1

[PATCH v6 11/12] platform/x86: simatic-ipc: drop custom P2SB bar code

[Andy Shevchenko]

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

The two drivers that used to use this have been switched over to the
common P2SB accessor, so this code is not needed any longer.

Signed-off-by: Henning Schild <henning.schild@siemens.com>
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/platform/x86/simatic-ipc.c            | 38 -------------------
 .../platform_data/x86/simatic-ipc-base.h      |  2 -
 2 files changed, 40 deletions(-)

diff --git a/drivers/platform/x86/simatic-ipc.c b/drivers/platform/x86/simatic-ipc.c
index b599cda5ba3c..26c35e1660cb 100644
--- a/drivers/platform/x86/simatic-ipc.c
+++ b/drivers/platform/x86/simatic-ipc.c
@@ -101,44 +101,6 @@ static int register_platform_devices(u32 station_id)
 	return 0;
 }
 
-/* FIXME: this should eventually be done with generic P2SB discovery code
- * the individual drivers for watchdogs and LEDs access memory that implements
- * GPIO, but pinctrl will not come up because of missing ACPI entries
- *
- * While there is no conflict a cleaner solution would be to somehow bring up
- * pinctrl even with these ACPI entries missing, and base the drivers on pinctrl.
- * After which the following function could be dropped, together with the code
- * poking the memory.
- */
-/*
- * Get membase address from PCI, used in leds and wdt module. Here we read
- * the bar0. The final address calculation is done in the appropriate modules
- */
-u32 simatic_ipc_get_membase0(unsigned int p2sb)
-{
-	struct pci_bus *bus;
-	u32 bar0 = 0;
-	/*
-	 * The GPIO memory is in bar0 of the hidden P2SB device.
-	 * Unhide the device to have a quick look at it, before we hide it
-	 * again.
-	 * Also grab the pci rescan lock so that device does not get discovered
-	 * and remapped while it is visible.
-	 * This code is inspired by drivers/mfd/lpc_ich.c
-	 */
-	bus = pci_find_bus(0, 0);
-	pci_lock_rescan_remove();
-	pci_bus_write_config_byte(bus, p2sb, 0xE1, 0x0);
-	pci_bus_read_config_dword(bus, p2sb, PCI_BASE_ADDRESS_0, &bar0);
-
-	bar0 &= ~0xf;
-	pci_bus_write_config_byte(bus, p2sb, 0xE1, 0x1);
-	pci_unlock_rescan_remove();
-
-	return bar0;
-}
-EXPORT_SYMBOL(simatic_ipc_get_membase0);
-
 static int __init simatic_ipc_init_module(void)
 {
 	const struct dmi_system_id *match;
diff --git a/include/linux/platform_data/x86/simatic-ipc-base.h b/include/linux/platform_data/x86/simatic-ipc-base.h
index 62d2bc774067..39fefd48cf4d 100644
--- a/include/linux/platform_data/x86/simatic-ipc-base.h
+++ b/include/linux/platform_data/x86/simatic-ipc-base.h
@@ -24,6 +24,4 @@ struct simatic_ipc_platform {
 	u8	devmode;
 };
 
-u32 simatic_ipc_get_membase0(unsigned int p2sb);
-
 #endif /* __PLATFORM_DATA_X86_SIMATIC_IPC_BASE_H */
-- 
2.35.1

Re: [PATCH v6 11/12] platform/x86: simatic-ipc: drop custom P2SB bar code

[Hans de Goede]

Hi,

On 6/6/22 18:41, Andy Shevchenko wrote:
> From: Henning Schild <henning.schild@siemens.com>
> 
> The two drivers that used to use this have been switched over to the
> common P2SB accessor, so this code is not needed any longer.
> 
> Signed-off-by: Henning Schild <henning.schild@siemens.com>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

Thanks, patch looks good to me:

Reviewed-by: Hans de Goede <hdegoede@redhat.com>

Also Ack for merging this through the MFD tree.

Regards,

Hans

> ---
>  drivers/platform/x86/simatic-ipc.c            | 38 -------------------
>  .../platform_data/x86/simatic-ipc-base.h      |  2 -
>  2 files changed, 40 deletions(-)
> 
> diff --git a/drivers/platform/x86/simatic-ipc.c b/drivers/platform/x86/simatic-ipc.c
> index b599cda5ba3c..26c35e1660cb 100644
> --- a/drivers/platform/x86/simatic-ipc.c
> +++ b/drivers/platform/x86/simatic-ipc.c
> @@ -101,44 +101,6 @@ static int register_platform_devices(u32 station_id)
>  	return 0;
>  }
>  
> -/* FIXME: this should eventually be done with generic P2SB discovery code
> - * the individual drivers for watchdogs and LEDs access memory that implements
> - * GPIO, but pinctrl will not come up because of missing ACPI entries
> - *
> - * While there is no conflict a cleaner solution would be to somehow bring up
> - * pinctrl even with these ACPI entries missing, and base the drivers on pinctrl.
> - * After which the following function could be dropped, together with the code
> - * poking the memory.
> - */
> -/*
> - * Get membase address from PCI, used in leds and wdt module. Here we read
> - * the bar0. The final address calculation is done in the appropriate modules
> - */
> -u32 simatic_ipc_get_membase0(unsigned int p2sb)
> -{
> -	struct pci_bus *bus;
> -	u32 bar0 = 0;
> -	/*
> -	 * The GPIO memory is in bar0 of the hidden P2SB device.
> -	 * Unhide the device to have a quick look at it, before we hide it
> -	 * again.
> -	 * Also grab the pci rescan lock so that device does not get discovered
> -	 * and remapped while it is visible.
> -	 * This code is inspired by drivers/mfd/lpc_ich.c
> -	 */
> -	bus = pci_find_bus(0, 0);
> -	pci_lock_rescan_remove();
> -	pci_bus_write_config_byte(bus, p2sb, 0xE1, 0x0);
> -	pci_bus_read_config_dword(bus, p2sb, PCI_BASE_ADDRESS_0, &bar0);
> -
> -	bar0 &= ~0xf;
> -	pci_bus_write_config_byte(bus, p2sb, 0xE1, 0x1);
> -	pci_unlock_rescan_remove();
> -
> -	return bar0;
> -}
> -EXPORT_SYMBOL(simatic_ipc_get_membase0);
> -
>  static int __init simatic_ipc_init_module(void)
>  {
>  	const struct dmi_system_id *match;
> diff --git a/include/linux/platform_data/x86/simatic-ipc-base.h b/include/linux/platform_data/x86/simatic-ipc-base.h
> index 62d2bc774067..39fefd48cf4d 100644
> --- a/include/linux/platform_data/x86/simatic-ipc-base.h
> +++ b/include/linux/platform_data/x86/simatic-ipc-base.h
> @@ -24,6 +24,4 @@ struct simatic_ipc_platform {
>  	u8	devmode;
>  };
>  
> -u32 simatic_ipc_get_membase0(unsigned int p2sb);
> -
>  #endif /* __PLATFORM_DATA_X86_SIMATIC_IPC_BASE_H */

[PATCH v6 12/12] leds: simatic-ipc-leds-gpio: add GPIO version of Siemens driver

[Andy Shevchenko]

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

On Apollo Lake the pinctrl drivers will now come up without ACPI. Use
that instead of open coding it.
Create a new driver for that which can later be filled with more GPIO
based models, and which has different dependencies.

Signed-off-by: Henning Schild <henning.schild@siemens.com>
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/leds/simple/Kconfig                 |   7 +-
 drivers/leds/simple/Makefile                |   1 +
 drivers/leds/simple/simatic-ipc-leds-gpio.c | 105 ++++++++++++++++++++
 drivers/leds/simple/simatic-ipc-leds.c      |  80 +--------------
 drivers/platform/x86/simatic-ipc.c          |   5 +-
 5 files changed, 117 insertions(+), 81 deletions(-)
 create mode 100644 drivers/leds/simple/simatic-ipc-leds-gpio.c

diff --git a/drivers/leds/simple/Kconfig b/drivers/leds/simple/Kconfig
index bbf8cff3c3f6..fd2b8225d926 100644
--- a/drivers/leds/simple/Kconfig
+++ b/drivers/leds/simple/Kconfig
@@ -1,12 +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 LEDS_GPIO
 	depends on SIEMENS_SIMATIC_IPC
-	select P2SB
 	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.
+	  To compile this driver as a module, choose M here: the modules
+	  will be called simatic-ipc-leds and simatic-ipc-leds-gpio.
diff --git a/drivers/leds/simple/Makefile b/drivers/leds/simple/Makefile
index 8481f1e9e360..1c7ef5e1324b 100644
--- a/drivers/leds/simple/Makefile
+++ b/drivers/leds/simple/Makefile
@@ -1,2 +1,3 @@
 # SPDX-License-Identifier: GPL-2.0
 obj-$(CONFIG_LEDS_SIEMENS_SIMATIC_IPC)	+= simatic-ipc-leds.o
+obj-$(CONFIG_LEDS_SIEMENS_SIMATIC_IPC)	+= simatic-ipc-leds-gpio.o
diff --git a/drivers/leds/simple/simatic-ipc-leds-gpio.c b/drivers/leds/simple/simatic-ipc-leds-gpio.c
new file mode 100644
index 000000000000..4c9e663a90ba
--- /dev/null
+++ b/drivers/leds/simple/simatic-ipc-leds-gpio.c
@@ -0,0 +1,105 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Siemens SIMATIC IPC driver for GPIO based LEDs
+ *
+ * Copyright (c) Siemens AG, 2022
+ *
+ * Authors:
+ *  Henning Schild <henning.schild@siemens.com>
+ */
+
+#include <linux/gpio/machine.h>
+#include <linux/gpio/consumer.h>
+#include <linux/leds.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+
+static struct gpiod_lookup_table simatic_ipc_led_gpio_table = {
+	.dev_id = "leds-gpio",
+	.table = {
+		GPIO_LOOKUP_IDX("apollolake-pinctrl.0", 51, NULL, 0, GPIO_ACTIVE_LOW),
+		GPIO_LOOKUP_IDX("apollolake-pinctrl.0", 52, NULL, 1, GPIO_ACTIVE_LOW),
+		GPIO_LOOKUP_IDX("apollolake-pinctrl.0", 53, NULL, 2, GPIO_ACTIVE_LOW),
+		GPIO_LOOKUP_IDX("apollolake-pinctrl.0", 57, NULL, 3, GPIO_ACTIVE_LOW),
+		GPIO_LOOKUP_IDX("apollolake-pinctrl.0", 58, NULL, 4, GPIO_ACTIVE_LOW),
+		GPIO_LOOKUP_IDX("apollolake-pinctrl.0", 60, NULL, 5, GPIO_ACTIVE_LOW),
+		GPIO_LOOKUP_IDX("apollolake-pinctrl.0", 56, NULL, 6, GPIO_ACTIVE_LOW),
+		GPIO_LOOKUP_IDX("apollolake-pinctrl.0", 59, NULL, 7, GPIO_ACTIVE_HIGH),
+	},
+};
+
+static const struct gpio_led simatic_ipc_gpio_leds[] = {
+	{ .name = "green:" LED_FUNCTION_STATUS "-3" },
+	{ .name = "red:" LED_FUNCTION_STATUS "-1" },
+	{ .name = "green:" LED_FUNCTION_STATUS "-1" },
+	{ .name = "red:" LED_FUNCTION_STATUS "-2" },
+	{ .name = "green:" LED_FUNCTION_STATUS "-2" },
+	{ .name = "red:" LED_FUNCTION_STATUS "-3" },
+};
+
+static const struct gpio_led_platform_data simatic_ipc_gpio_leds_pdata = {
+	.num_leds	= ARRAY_SIZE(simatic_ipc_gpio_leds),
+	.leds		= simatic_ipc_gpio_leds,
+};
+
+static struct platform_device *simatic_leds_pdev;
+
+static int simatic_ipc_leds_gpio_remove(struct platform_device *pdev)
+{
+	gpiod_remove_lookup_table(&simatic_ipc_led_gpio_table);
+	platform_device_unregister(simatic_leds_pdev);
+
+	return 0;
+}
+
+static int simatic_ipc_leds_gpio_probe(struct platform_device *pdev)
+{
+	struct gpio_desc *gpiod;
+	int err;
+
+	gpiod_add_lookup_table(&simatic_ipc_led_gpio_table);
+	simatic_leds_pdev = platform_device_register_resndata(NULL,
+		"leds-gpio", PLATFORM_DEVID_NONE, NULL, 0,
+		&simatic_ipc_gpio_leds_pdata,
+		sizeof(simatic_ipc_gpio_leds_pdata));
+	if (IS_ERR(simatic_leds_pdev)) {
+		err = PTR_ERR(simatic_leds_pdev);
+		goto out;
+	}
+
+	/* PM_BIOS_BOOT_N */
+	gpiod = gpiod_get_index(&simatic_leds_pdev->dev, NULL, 6, GPIOD_OUT_LOW);
+	if (IS_ERR(gpiod)) {
+		err = PTR_ERR(gpiod);
+		goto out;
+	}
+	gpiod_put(gpiod);
+
+	/* PM_WDT_OUT */
+	gpiod = gpiod_get_index(&simatic_leds_pdev->dev, NULL, 7, GPIOD_OUT_LOW);
+	if (IS_ERR(gpiod)) {
+		err = PTR_ERR(gpiod);
+		goto out;
+	}
+	gpiod_put(gpiod);
+
+	return 0;
+out:
+	simatic_ipc_leds_gpio_remove(pdev);
+
+	return err;
+}
+
+static struct platform_driver simatic_ipc_led_gpio_driver = {
+	.probe = simatic_ipc_leds_gpio_probe,
+	.remove = simatic_ipc_leds_gpio_remove,
+	.driver = {
+		.name = KBUILD_MODNAME,
+	}
+};
+module_platform_driver(simatic_ipc_led_gpio_driver);
+
+MODULE_LICENSE("GPL v2");
+MODULE_ALIAS("platform:" KBUILD_MODNAME);
+MODULE_SOFTDEP("pre: platform:leds-gpio");
+MODULE_AUTHOR("Henning Schild <henning.schild@siemens.com>");
diff --git a/drivers/leds/simple/simatic-ipc-leds.c b/drivers/leds/simple/simatic-ipc-leds.c
index 2e7597c143d8..4894c228c165 100644
--- a/drivers/leds/simple/simatic-ipc-leds.c
+++ b/drivers/leds/simple/simatic-ipc-leds.c
@@ -15,7 +15,6 @@
 #include <linux/leds.h>
 #include <linux/module.h>
 #include <linux/pci.h>
-#include <linux/platform_data/x86/p2sb.h>
 #include <linux/platform_data/x86/simatic-ipc-base.h>
 #include <linux/platform_device.h>
 #include <linux/sizes.h>
@@ -24,7 +23,7 @@
 #define SIMATIC_IPC_LED_PORT_BASE	0x404E
 
 struct simatic_ipc_led {
-	unsigned int value; /* mask for io and offset for mem */
+	unsigned int value; /* mask for io */
 	char *name;
 	struct led_classdev cdev;
 };
@@ -39,21 +38,6 @@ static struct simatic_ipc_led simatic_ipc_leds_io[] = {
 	{ }
 };
 
-/* the actual start will be discovered with p2sb, 0 is a placeholder */
-static struct resource simatic_ipc_led_mem_res = DEFINE_RES_MEM_NAMED(0, 0, KBUILD_MODNAME);
-
-static void __iomem *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_2, KBUILD_MODNAME);
 
@@ -89,28 +73,6 @@ static enum led_brightness simatic_ipc_led_get_io(struct led_classdev *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);
-	void __iomem *reg = simatic_ipc_led_memory + led->value;
-	u32 val;
-
-	val = readl(reg);
-	val = (val & ~1) | (brightness == LED_OFF);
-	writel(val, reg);
-}
-
-static enum led_brightness simatic_ipc_led_get_mem(struct led_classdev *led_cd)
-{
-	struct simatic_ipc_led *led = cdev_to_led(led_cd);
-	void __iomem *reg = simatic_ipc_led_memory + led->value;
-	u32 val;
-
-	val = readl(reg);
-	return (val & 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;
@@ -118,9 +80,7 @@ static int simatic_ipc_leds_probe(struct platform_device *pdev)
 	struct simatic_ipc_led *ipcled;
 	struct led_classdev *cdev;
 	struct resource *res;
-	void __iomem *reg;
-	int err, type;
-	u32 val;
+	int err;
 
 	switch (plat->devmode) {
 	case SIMATIC_IPC_DEVICE_227D:
@@ -135,51 +95,19 @@ static int simatic_ipc_leds_probe(struct platform_device *pdev)
 			}
 			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;
-
-		err = p2sb_bar(NULL, 0, res);
-		if (err)
-			return err;
-
-		/* do the final address calculation */
-		res->start = res->start + (0xC5 << 16);
-		res->end = res->start + SZ_4K - 1;
-
-		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 */
-		reg = simatic_ipc_led_memory + 0x500 + 0x1D8; /* PM_WDT_OUT */
-		val = readl(reg);
-		writel(val & ~1, reg);
-
-		reg = simatic_ipc_led_memory + 0x500 + 0x1C0; /* PM_BIOS_BOOT_N */
-		val = readl(reg);
-		writel(val | 1, reg);
-		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->brightness_set = simatic_ipc_led_set_io;
+		cdev->brightness_get = simatic_ipc_led_get_io;
 		cdev->max_brightness = LED_ON;
 		cdev->name = ipcled->name;
 
diff --git a/drivers/platform/x86/simatic-ipc.c b/drivers/platform/x86/simatic-ipc.c
index 26c35e1660cb..ca3647b751d5 100644
--- a/drivers/platform/x86/simatic-ipc.c
+++ b/drivers/platform/x86/simatic-ipc.c
@@ -51,6 +51,7 @@ static int register_platform_devices(u32 station_id)
 {
 	u8 ledmode = SIMATIC_IPC_DEVICE_NONE;
 	u8 wdtmode = SIMATIC_IPC_DEVICE_NONE;
+	char *pdevname = KBUILD_MODNAME "_leds";
 	int i;
 
 	platform_data.devmode = SIMATIC_IPC_DEVICE_NONE;
@@ -64,10 +65,12 @@ static int register_platform_devices(u32 station_id)
 	}
 
 	if (ledmode != SIMATIC_IPC_DEVICE_NONE) {
+		if (ledmode == SIMATIC_IPC_DEVICE_127E)
+			pdevname = KBUILD_MODNAME "_leds_gpio";
 		platform_data.devmode = ledmode;
 		ipc_led_platform_device =
 			platform_device_register_data(NULL,
-				KBUILD_MODNAME "_leds", PLATFORM_DEVID_NONE,
+				pdevname, PLATFORM_DEVID_NONE,
 				&platform_data,
 				sizeof(struct simatic_ipc_platform));
 		if (IS_ERR(ipc_led_platform_device))
-- 
2.35.1

Re: [PATCH v6 12/12] leds: simatic-ipc-leds-gpio: add GPIO version of Siemens driver

[Henning Schild]

Am Mon,  6 Jun 2022 19:41:38 +0300
schrieb Andy Shevchenko <andriy.shevchenko@linux.intel.com>:

> From: Henning Schild <henning.schild@siemens.com>
> 
> On Apollo Lake the pinctrl drivers will now come up without ACPI. Use
> that instead of open coding it.
> Create a new driver for that which can later be filled with more GPIO
> based models, and which has different dependencies.
> 
> Signed-off-by: Henning Schild <henning.schild@siemens.com>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  drivers/leds/simple/Kconfig                 |   7 +-
>  drivers/leds/simple/Makefile                |   1 +
>  drivers/leds/simple/simatic-ipc-leds-gpio.c | 105
> ++++++++++++++++++++ drivers/leds/simple/simatic-ipc-leds.c      |
> 80 +-------------- drivers/platform/x86/simatic-ipc.c          |   5
> +- 5 files changed, 117 insertions(+), 81 deletions(-)
>  create mode 100644 drivers/leds/simple/simatic-ipc-leds-gpio.c
> 
> diff --git a/drivers/leds/simple/Kconfig b/drivers/leds/simple/Kconfig
> index bbf8cff3c3f6..fd2b8225d926 100644
> --- a/drivers/leds/simple/Kconfig
> +++ b/drivers/leds/simple/Kconfig
> @@ -1,12 +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 LEDS_GPIO
>  	depends on SIEMENS_SIMATIC_IPC
> -	select P2SB
>  	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.
> +	  To compile this driver as a module, choose M here: the
> modules
> +	  will be called simatic-ipc-leds and simatic-ipc-leds-gpio.
> diff --git a/drivers/leds/simple/Makefile
> b/drivers/leds/simple/Makefile index 8481f1e9e360..1c7ef5e1324b 100644
> --- a/drivers/leds/simple/Makefile
> +++ b/drivers/leds/simple/Makefile
> @@ -1,2 +1,3 @@
>  # SPDX-License-Identifier: GPL-2.0
>  obj-$(CONFIG_LEDS_SIEMENS_SIMATIC_IPC)	+= simatic-ipc-leds.o
> +obj-$(CONFIG_LEDS_SIEMENS_SIMATIC_IPC)	+=
> simatic-ipc-leds-gpio.o diff --git
> a/drivers/leds/simple/simatic-ipc-leds-gpio.c
> b/drivers/leds/simple/simatic-ipc-leds-gpio.c new file mode 100644
> index 000000000000..4c9e663a90ba --- /dev/null
> +++ b/drivers/leds/simple/simatic-ipc-leds-gpio.c
> @@ -0,0 +1,105 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Siemens SIMATIC IPC driver for GPIO based LEDs
> + *
> + * Copyright (c) Siemens AG, 2022
> + *
> + * Authors:
> + *  Henning Schild <henning.schild@siemens.com>
> + */
> +
> +#include <linux/gpio/machine.h>
> +#include <linux/gpio/consumer.h>
> +#include <linux/leds.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +
> +static struct gpiod_lookup_table simatic_ipc_led_gpio_table = {
> +	.dev_id = "leds-gpio",
> +	.table = {
> +		GPIO_LOOKUP_IDX("apollolake-pinctrl.0", 51, NULL, 0,
> GPIO_ACTIVE_LOW),
> +		GPIO_LOOKUP_IDX("apollolake-pinctrl.0", 52, NULL, 1,
> GPIO_ACTIVE_LOW),
> +		GPIO_LOOKUP_IDX("apollolake-pinctrl.0", 53, NULL, 2,
> GPIO_ACTIVE_LOW),
> +		GPIO_LOOKUP_IDX("apollolake-pinctrl.0", 57, NULL, 3,
> GPIO_ACTIVE_LOW),
> +		GPIO_LOOKUP_IDX("apollolake-pinctrl.0", 58, NULL, 4,
> GPIO_ACTIVE_LOW),
> +		GPIO_LOOKUP_IDX("apollolake-pinctrl.0", 60, NULL, 5,
> GPIO_ACTIVE_LOW),
> +		GPIO_LOOKUP_IDX("apollolake-pinctrl.0", 56, NULL, 6,
> GPIO_ACTIVE_LOW),
> +		GPIO_LOOKUP_IDX("apollolake-pinctrl.0", 59, NULL, 7,
> GPIO_ACTIVE_HIGH),
> +	},
> +};
> +
> +static const struct gpio_led simatic_ipc_gpio_leds[] = {
> +	{ .name = "green:" LED_FUNCTION_STATUS "-3" },
> +	{ .name = "red:" LED_FUNCTION_STATUS "-1" },
> +	{ .name = "green:" LED_FUNCTION_STATUS "-1" },
> +	{ .name = "red:" LED_FUNCTION_STATUS "-2" },
> +	{ .name = "green:" LED_FUNCTION_STATUS "-2" },
> +	{ .name = "red:" LED_FUNCTION_STATUS "-3" },
> +};
> +
> +static const struct gpio_led_platform_data
> simatic_ipc_gpio_leds_pdata = {
> +	.num_leds	= ARRAY_SIZE(simatic_ipc_gpio_leds),
> +	.leds		= simatic_ipc_gpio_leds,
> +};
> +
> +static struct platform_device *simatic_leds_pdev;
> +
> +static int simatic_ipc_leds_gpio_remove(struct platform_device *pdev)
> +{
> +	gpiod_remove_lookup_table(&simatic_ipc_led_gpio_table);
> +	platform_device_unregister(simatic_leds_pdev);
> +
> +	return 0;
> +}
> +
> +static int simatic_ipc_leds_gpio_probe(struct platform_device *pdev)
> +{
> +	struct gpio_desc *gpiod;
> +	int err;
> +
> +	gpiod_add_lookup_table(&simatic_ipc_led_gpio_table);
> +	simatic_leds_pdev = platform_device_register_resndata(NULL,
> +		"leds-gpio", PLATFORM_DEVID_NONE, NULL, 0,
> +		&simatic_ipc_gpio_leds_pdata,
> +		sizeof(simatic_ipc_gpio_leds_pdata));

If those GPIOs can not be found that leads to a pretty severe
logging/polling endless loop.

...
[   34.017017] leds-gpio leds-gpio: Skipping unavailable LED gpio 0 (red:status-2)
[   34.017038] leds-gpio leds-gpio: cannot find GPIO chip apollolake-pinctrl.0, deferring
[   34.017158] leds-gpio leds-gpio: Skipping unavailable LED gpio 0 (red:status-2)
[   34.017161] leds-gpio leds-gpio: cannot find GPIO chip apollolake-pinctrl.0, deferring
[   34.017163] leds-gpio leds-gpio: Skipping unavailable LED gpio 0 (green:status-2)
[   34.017166] leds-gpio leds-gpio: cannot find GPIO chip apollolake-pinctrl.0, deferring
[   34.017168] leds-gpio leds-gpio: Skipping unavailable LED gpio 0 (red:status-3)
[   34.017179] leds-gpio leds-gpio: cannot find GPIO chip apollolake-pinctrl.0, deferring
[   34.017275] leds-gpio leds-gpio: cannot find GPIO chip apollolake-pinctrl.0, deferring
[   34.017279] leds-gpio leds-gpio: Skipping unavailable LED gpio 0 (green:status-3)
... 

This can be seen when the kernel is build without
CONFIG_PINCTRL_BROXTON. Can anyone please give advise on how to proceed
here.

I could do something like i proposed in "[PATCH 3/4] leds:
simatic-ipc-leds-gpio: add new model 227G".

if (!IS_ENABLED(CONFIG_PINCTRL_BROXTON)) return -ENO...

before registering the platform device.

An alternative could be to try and handle that -EPROBE_DEFER but that
might be hard because it might well fail a couple of times before
eventually that gpio driver is up, so i think i will send an update
implementing that !IS_ENABLED.

Ideally the gpio subsystem or leds-gpio would break that
polling/printing loop for me, or make sure it would not happen in the
first place.

regards,
Henning

> +	if (IS_ERR(simatic_leds_pdev)) {
> +		err = PTR_ERR(simatic_leds_pdev);
> +		goto out;
> +	}
> +
> +	/* PM_BIOS_BOOT_N */
> +	gpiod = gpiod_get_index(&simatic_leds_pdev->dev, NULL, 6,
> GPIOD_OUT_LOW);
> +	if (IS_ERR(gpiod)) {
> +		err = PTR_ERR(gpiod);
> +		goto out;
> +	}
> +	gpiod_put(gpiod);
> +
> +	/* PM_WDT_OUT */
> +	gpiod = gpiod_get_index(&simatic_leds_pdev->dev, NULL, 7,
> GPIOD_OUT_LOW);
> +	if (IS_ERR(gpiod)) {
> +		err = PTR_ERR(gpiod);
> +		goto out;
> +	}
> +	gpiod_put(gpiod);
> +
> +	return 0;
> +out:
> +	simatic_ipc_leds_gpio_remove(pdev);
> +
> +	return err;
> +}
> +
> +static struct platform_driver simatic_ipc_led_gpio_driver = {
> +	.probe = simatic_ipc_leds_gpio_probe,
> +	.remove = simatic_ipc_leds_gpio_remove,
> +	.driver = {
> +		.name = KBUILD_MODNAME,
> +	}
> +};
> +module_platform_driver(simatic_ipc_led_gpio_driver);
> +
> +MODULE_LICENSE("GPL v2");
> +MODULE_ALIAS("platform:" KBUILD_MODNAME);
> +MODULE_SOFTDEP("pre: platform:leds-gpio");
> +MODULE_AUTHOR("Henning Schild <henning.schild@siemens.com>");
> diff --git a/drivers/leds/simple/simatic-ipc-leds.c
> b/drivers/leds/simple/simatic-ipc-leds.c index
> 2e7597c143d8..4894c228c165 100644 ---
> a/drivers/leds/simple/simatic-ipc-leds.c +++
> b/drivers/leds/simple/simatic-ipc-leds.c @@ -15,7 +15,6 @@
>  #include <linux/leds.h>
>  #include <linux/module.h>
>  #include <linux/pci.h>
> -#include <linux/platform_data/x86/p2sb.h>
>  #include <linux/platform_data/x86/simatic-ipc-base.h>
>  #include <linux/platform_device.h>
>  #include <linux/sizes.h>
> @@ -24,7 +23,7 @@
>  #define SIMATIC_IPC_LED_PORT_BASE	0x404E
>  
>  struct simatic_ipc_led {
> -	unsigned int value; /* mask for io and offset for mem */
> +	unsigned int value; /* mask for io */
>  	char *name;
>  	struct led_classdev cdev;
>  };
> @@ -39,21 +38,6 @@ static struct simatic_ipc_led
> simatic_ipc_leds_io[] = { { }
>  };
>  
> -/* the actual start will be discovered with p2sb, 0 is a placeholder
> */ -static struct resource simatic_ipc_led_mem_res =
> DEFINE_RES_MEM_NAMED(0, 0, KBUILD_MODNAME); -
> -static void __iomem *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_2,
> KBUILD_MODNAME); 
> @@ -89,28 +73,6 @@ static enum led_brightness
> simatic_ipc_led_get_io(struct led_classdev *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);
> -	void __iomem *reg = simatic_ipc_led_memory + led->value;
> -	u32 val;
> -
> -	val = readl(reg);
> -	val = (val & ~1) | (brightness == LED_OFF);
> -	writel(val, reg);
> -}
> -
> -static enum led_brightness simatic_ipc_led_get_mem(struct
> led_classdev *led_cd) -{
> -	struct simatic_ipc_led *led = cdev_to_led(led_cd);
> -	void __iomem *reg = simatic_ipc_led_memory + led->value;
> -	u32 val;
> -
> -	val = readl(reg);
> -	return (val & 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; @@ -118,9 +80,7 @@ static int
> simatic_ipc_leds_probe(struct platform_device *pdev) struct
> simatic_ipc_led *ipcled; struct led_classdev *cdev;
>  	struct resource *res;
> -	void __iomem *reg;
> -	int err, type;
> -	u32 val;
> +	int err;
>  
>  	switch (plat->devmode) {
>  	case SIMATIC_IPC_DEVICE_227D:
> @@ -135,51 +95,19 @@ static int simatic_ipc_leds_probe(struct
> platform_device *pdev) }
>  			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;
> -
> -		err = p2sb_bar(NULL, 0, res);
> -		if (err)
> -			return err;
> -
> -		/* do the final address calculation */
> -		res->start = res->start + (0xC5 << 16);
> -		res->end = res->start + SZ_4K - 1;
> -
> -		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 */
> -		reg = simatic_ipc_led_memory + 0x500 + 0x1D8; /*
> PM_WDT_OUT */
> -		val = readl(reg);
> -		writel(val & ~1, reg);
> -
> -		reg = simatic_ipc_led_memory + 0x500 + 0x1C0; /*
> PM_BIOS_BOOT_N */
> -		val = readl(reg);
> -		writel(val | 1, reg);
> -		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->brightness_set = simatic_ipc_led_set_io;
> +		cdev->brightness_get = simatic_ipc_led_get_io;
>  		cdev->max_brightness = LED_ON;
>  		cdev->name = ipcled->name;
>  
> diff --git a/drivers/platform/x86/simatic-ipc.c
> b/drivers/platform/x86/simatic-ipc.c index 26c35e1660cb..ca3647b751d5
> 100644 --- a/drivers/platform/x86/simatic-ipc.c
> +++ b/drivers/platform/x86/simatic-ipc.c
> @@ -51,6 +51,7 @@ static int register_platform_devices(u32 station_id)
>  {
>  	u8 ledmode = SIMATIC_IPC_DEVICE_NONE;
>  	u8 wdtmode = SIMATIC_IPC_DEVICE_NONE;
> +	char *pdevname = KBUILD_MODNAME "_leds";
>  	int i;
>  
>  	platform_data.devmode = SIMATIC_IPC_DEVICE_NONE;
> @@ -64,10 +65,12 @@ static int register_platform_devices(u32
> station_id) }
>  
>  	if (ledmode != SIMATIC_IPC_DEVICE_NONE) {
> +		if (ledmode == SIMATIC_IPC_DEVICE_127E)
> +			pdevname = KBUILD_MODNAME "_leds_gpio";
>  		platform_data.devmode = ledmode;
>  		ipc_led_platform_device =
>  			platform_device_register_data(NULL,
> -				KBUILD_MODNAME "_leds",
> PLATFORM_DEVID_NONE,
> +				pdevname, PLATFORM_DEVID_NONE,
>  				&platform_data,
>  				sizeof(struct simatic_ipc_platform));
>  		if (IS_ERR(ipc_led_platform_device))

Re: [PATCH v6 12/12] leds: simatic-ipc-leds-gpio: add GPIO version of Siemens driver

[Henning Schild]

Am Mon, 1 Aug 2022 16:09:48 +0200
schrieb Henning Schild <henning.schild@siemens.com>:

> Am Mon,  6 Jun 2022 19:41:38 +0300
> schrieb Andy Shevchenko <andriy.shevchenko@linux.intel.com>:
> 
> > From: Henning Schild <henning.schild@siemens.com>
> > 
> > On Apollo Lake the pinctrl drivers will now come up without ACPI.
> > Use that instead of open coding it.
> > Create a new driver for that which can later be filled with more
> > GPIO based models, and which has different dependencies.
> > 
> > Signed-off-by: Henning Schild <henning.schild@siemens.com>
> > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > ---
> >  drivers/leds/simple/Kconfig                 |   7 +-
> >  drivers/leds/simple/Makefile                |   1 +
> >  drivers/leds/simple/simatic-ipc-leds-gpio.c | 105
> > ++++++++++++++++++++ drivers/leds/simple/simatic-ipc-leds.c      |
> > 80 +-------------- drivers/platform/x86/simatic-ipc.c          |   5
> > +- 5 files changed, 117 insertions(+), 81 deletions(-)
> >  create mode 100644 drivers/leds/simple/simatic-ipc-leds-gpio.c
> > 
> > diff --git a/drivers/leds/simple/Kconfig
> > b/drivers/leds/simple/Kconfig index bbf8cff3c3f6..fd2b8225d926
> > 100644 --- a/drivers/leds/simple/Kconfig
> > +++ b/drivers/leds/simple/Kconfig
> > @@ -1,12 +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 LEDS_GPIO

depends on PINCTRL_BROXTON

Will have to see how to handle the additional boards. Either a gpio
driver per pinctrl provider with a clear dep chain, or some sort up
runtime check like i described in the earlier mail. Or simply depend on
all potential providers in one driver, even if on a given board only
one will be used.

Henning

> >  	depends on SIEMENS_SIMATIC_IPC
> > -	select P2SB
> >  	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.
> > +	  To compile this driver as a module, choose M here: the
> > modules
> > +	  will be called simatic-ipc-leds and
> > simatic-ipc-leds-gpio. diff --git a/drivers/leds/simple/Makefile
> > b/drivers/leds/simple/Makefile index 8481f1e9e360..1c7ef5e1324b
> > 100644 --- a/drivers/leds/simple/Makefile
> > +++ b/drivers/leds/simple/Makefile
> > @@ -1,2 +1,3 @@
> >  # SPDX-License-Identifier: GPL-2.0
> >  obj-$(CONFIG_LEDS_SIEMENS_SIMATIC_IPC)	+= simatic-ipc-leds.o
> > +obj-$(CONFIG_LEDS_SIEMENS_SIMATIC_IPC)	+=
> > simatic-ipc-leds-gpio.o diff --git
> > a/drivers/leds/simple/simatic-ipc-leds-gpio.c
> > b/drivers/leds/simple/simatic-ipc-leds-gpio.c new file mode 100644
> > index 000000000000..4c9e663a90ba --- /dev/null
> > +++ b/drivers/leds/simple/simatic-ipc-leds-gpio.c
> > @@ -0,0 +1,105 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Siemens SIMATIC IPC driver for GPIO based LEDs
> > + *
> > + * Copyright (c) Siemens AG, 2022
> > + *
> > + * Authors:
> > + *  Henning Schild <henning.schild@siemens.com>
> > + */
> > +
> > +#include <linux/gpio/machine.h>
> > +#include <linux/gpio/consumer.h>
> > +#include <linux/leds.h>
> > +#include <linux/module.h>
> > +#include <linux/platform_device.h>
> > +
> > +static struct gpiod_lookup_table simatic_ipc_led_gpio_table = {
> > +	.dev_id = "leds-gpio",
> > +	.table = {
> > +		GPIO_LOOKUP_IDX("apollolake-pinctrl.0", 51, NULL,
> > 0, GPIO_ACTIVE_LOW),
> > +		GPIO_LOOKUP_IDX("apollolake-pinctrl.0", 52, NULL,
> > 1, GPIO_ACTIVE_LOW),
> > +		GPIO_LOOKUP_IDX("apollolake-pinctrl.0", 53, NULL,
> > 2, GPIO_ACTIVE_LOW),
> > +		GPIO_LOOKUP_IDX("apollolake-pinctrl.0", 57, NULL,
> > 3, GPIO_ACTIVE_LOW),
> > +		GPIO_LOOKUP_IDX("apollolake-pinctrl.0", 58, NULL,
> > 4, GPIO_ACTIVE_LOW),
> > +		GPIO_LOOKUP_IDX("apollolake-pinctrl.0", 60, NULL,
> > 5, GPIO_ACTIVE_LOW),
> > +		GPIO_LOOKUP_IDX("apollolake-pinctrl.0", 56, NULL,
> > 6, GPIO_ACTIVE_LOW),
> > +		GPIO_LOOKUP_IDX("apollolake-pinctrl.0", 59, NULL,
> > 7, GPIO_ACTIVE_HIGH),
> > +	},
> > +};
> > +
> > +static const struct gpio_led simatic_ipc_gpio_leds[] = {
> > +	{ .name = "green:" LED_FUNCTION_STATUS "-3" },
> > +	{ .name = "red:" LED_FUNCTION_STATUS "-1" },
> > +	{ .name = "green:" LED_FUNCTION_STATUS "-1" },
> > +	{ .name = "red:" LED_FUNCTION_STATUS "-2" },
> > +	{ .name = "green:" LED_FUNCTION_STATUS "-2" },
> > +	{ .name = "red:" LED_FUNCTION_STATUS "-3" },
> > +};
> > +
> > +static const struct gpio_led_platform_data
> > simatic_ipc_gpio_leds_pdata = {
> > +	.num_leds	= ARRAY_SIZE(simatic_ipc_gpio_leds),
> > +	.leds		= simatic_ipc_gpio_leds,
> > +};
> > +
> > +static struct platform_device *simatic_leds_pdev;
> > +
> > +static int simatic_ipc_leds_gpio_remove(struct platform_device
> > *pdev) +{
> > +	gpiod_remove_lookup_table(&simatic_ipc_led_gpio_table);
> > +	platform_device_unregister(simatic_leds_pdev);
> > +
> > +	return 0;
> > +}
> > +
> > +static int simatic_ipc_leds_gpio_probe(struct platform_device
> > *pdev) +{
> > +	struct gpio_desc *gpiod;
> > +	int err;
> > +
> > +	gpiod_add_lookup_table(&simatic_ipc_led_gpio_table);
> > +	simatic_leds_pdev = platform_device_register_resndata(NULL,
> > +		"leds-gpio", PLATFORM_DEVID_NONE, NULL, 0,
> > +		&simatic_ipc_gpio_leds_pdata,
> > +		sizeof(simatic_ipc_gpio_leds_pdata));  
> 
> If those GPIOs can not be found that leads to a pretty severe
> logging/polling endless loop.
> 
> ...
> [   34.017017] leds-gpio leds-gpio: Skipping unavailable LED gpio 0
> (red:status-2) [   34.017038] leds-gpio leds-gpio: cannot find GPIO
> chip apollolake-pinctrl.0, deferring [   34.017158] leds-gpio
> leds-gpio: Skipping unavailable LED gpio 0 (red:status-2) [
> 34.017161] leds-gpio leds-gpio: cannot find GPIO chip
> apollolake-pinctrl.0, deferring [   34.017163] leds-gpio leds-gpio:
> Skipping unavailable LED gpio 0 (green:status-2) [   34.017166]
> leds-gpio leds-gpio: cannot find GPIO chip apollolake-pinctrl.0,
> deferring [   34.017168] leds-gpio leds-gpio: Skipping unavailable
> LED gpio 0 (red:status-3) [   34.017179] leds-gpio leds-gpio: cannot
> find GPIO chip apollolake-pinctrl.0, deferring [   34.017275]
> leds-gpio leds-gpio: cannot find GPIO chip apollolake-pinctrl.0,
> deferring [   34.017279] leds-gpio leds-gpio: Skipping unavailable
> LED gpio 0 (green:status-3) ... 
> 
> This can be seen when the kernel is build without
> CONFIG_PINCTRL_BROXTON. Can anyone please give advise on how to
> proceed here.
> 
> I could do something like i proposed in "[PATCH 3/4] leds:
> simatic-ipc-leds-gpio: add new model 227G".
> 
> if (!IS_ENABLED(CONFIG_PINCTRL_BROXTON)) return -ENO...
> 
> before registering the platform device.
> 
> An alternative could be to try and handle that -EPROBE_DEFER but that
> might be hard because it might well fail a couple of times before
> eventually that gpio driver is up, so i think i will send an update
> implementing that !IS_ENABLED.
> 
> Ideally the gpio subsystem or leds-gpio would break that
> polling/printing loop for me, or make sure it would not happen in the
> first place.
> 
> regards,
> Henning
> 
> > +	if (IS_ERR(simatic_leds_pdev)) {
> > +		err = PTR_ERR(simatic_leds_pdev);
> > +		goto out;
> > +	}
> > +
> > +	/* PM_BIOS_BOOT_N */
> > +	gpiod = gpiod_get_index(&simatic_leds_pdev->dev, NULL, 6,
> > GPIOD_OUT_LOW);
> > +	if (IS_ERR(gpiod)) {
> > +		err = PTR_ERR(gpiod);
> > +		goto out;
> > +	}
> > +	gpiod_put(gpiod);
> > +
> > +	/* PM_WDT_OUT */
> > +	gpiod = gpiod_get_index(&simatic_leds_pdev->dev, NULL, 7,
> > GPIOD_OUT_LOW);
> > +	if (IS_ERR(gpiod)) {
> > +		err = PTR_ERR(gpiod);
> > +		goto out;
> > +	}
> > +	gpiod_put(gpiod);
> > +
> > +	return 0;
> > +out:
> > +	simatic_ipc_leds_gpio_remove(pdev);
> > +
> > +	return err;
> > +}
> > +
> > +static struct platform_driver simatic_ipc_led_gpio_driver = {
> > +	.probe = simatic_ipc_leds_gpio_probe,
> > +	.remove = simatic_ipc_leds_gpio_remove,
> > +	.driver = {
> > +		.name = KBUILD_MODNAME,
> > +	}
> > +};
> > +module_platform_driver(simatic_ipc_led_gpio_driver);
> > +
> > +MODULE_LICENSE("GPL v2");
> > +MODULE_ALIAS("platform:" KBUILD_MODNAME);
> > +MODULE_SOFTDEP("pre: platform:leds-gpio");
> > +MODULE_AUTHOR("Henning Schild <henning.schild@siemens.com>");
> > diff --git a/drivers/leds/simple/simatic-ipc-leds.c
> > b/drivers/leds/simple/simatic-ipc-leds.c index
> > 2e7597c143d8..4894c228c165 100644 ---
> > a/drivers/leds/simple/simatic-ipc-leds.c +++
> > b/drivers/leds/simple/simatic-ipc-leds.c @@ -15,7 +15,6 @@
> >  #include <linux/leds.h>
> >  #include <linux/module.h>
> >  #include <linux/pci.h>
> > -#include <linux/platform_data/x86/p2sb.h>
> >  #include <linux/platform_data/x86/simatic-ipc-base.h>
> >  #include <linux/platform_device.h>
> >  #include <linux/sizes.h>
> > @@ -24,7 +23,7 @@
> >  #define SIMATIC_IPC_LED_PORT_BASE	0x404E
> >  
> >  struct simatic_ipc_led {
> > -	unsigned int value; /* mask for io and offset for mem */
> > +	unsigned int value; /* mask for io */
> >  	char *name;
> >  	struct led_classdev cdev;
> >  };
> > @@ -39,21 +38,6 @@ static struct simatic_ipc_led
> > simatic_ipc_leds_io[] = { { }
> >  };
> >  
> > -/* the actual start will be discovered with p2sb, 0 is a
> > placeholder */ -static struct resource simatic_ipc_led_mem_res =
> > DEFINE_RES_MEM_NAMED(0, 0, KBUILD_MODNAME); -
> > -static void __iomem *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_2,
> > KBUILD_MODNAME); 
> > @@ -89,28 +73,6 @@ static enum led_brightness
> > simatic_ipc_led_get_io(struct led_classdev *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);
> > -	void __iomem *reg = simatic_ipc_led_memory + led->value;
> > -	u32 val;
> > -
> > -	val = readl(reg);
> > -	val = (val & ~1) | (brightness == LED_OFF);
> > -	writel(val, reg);
> > -}
> > -
> > -static enum led_brightness simatic_ipc_led_get_mem(struct
> > led_classdev *led_cd) -{
> > -	struct simatic_ipc_led *led = cdev_to_led(led_cd);
> > -	void __iomem *reg = simatic_ipc_led_memory + led->value;
> > -	u32 val;
> > -
> > -	val = readl(reg);
> > -	return (val & 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; @@ -118,9 +80,7 @@ static int
> > simatic_ipc_leds_probe(struct platform_device *pdev) struct
> > simatic_ipc_led *ipcled; struct led_classdev *cdev;
> >  	struct resource *res;
> > -	void __iomem *reg;
> > -	int err, type;
> > -	u32 val;
> > +	int err;
> >  
> >  	switch (plat->devmode) {
> >  	case SIMATIC_IPC_DEVICE_227D:
> > @@ -135,51 +95,19 @@ static int simatic_ipc_leds_probe(struct
> > platform_device *pdev) }
> >  			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;
> > -
> > -		err = p2sb_bar(NULL, 0, res);
> > -		if (err)
> > -			return err;
> > -
> > -		/* do the final address calculation */
> > -		res->start = res->start + (0xC5 << 16);
> > -		res->end = res->start + SZ_4K - 1;
> > -
> > -		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 */
> > -		reg = simatic_ipc_led_memory + 0x500 + 0x1D8; /*
> > PM_WDT_OUT */
> > -		val = readl(reg);
> > -		writel(val & ~1, reg);
> > -
> > -		reg = simatic_ipc_led_memory + 0x500 + 0x1C0; /*
> > PM_BIOS_BOOT_N */
> > -		val = readl(reg);
> > -		writel(val | 1, reg);
> > -		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->brightness_set = simatic_ipc_led_set_io;
> > +		cdev->brightness_get = simatic_ipc_led_get_io;
> >  		cdev->max_brightness = LED_ON;
> >  		cdev->name = ipcled->name;
> >  
> > diff --git a/drivers/platform/x86/simatic-ipc.c
> > b/drivers/platform/x86/simatic-ipc.c index
> > 26c35e1660cb..ca3647b751d5 100644 ---
> > a/drivers/platform/x86/simatic-ipc.c +++
> > b/drivers/platform/x86/simatic-ipc.c @@ -51,6 +51,7 @@ static int
> > register_platform_devices(u32 station_id) {
> >  	u8 ledmode = SIMATIC_IPC_DEVICE_NONE;
> >  	u8 wdtmode = SIMATIC_IPC_DEVICE_NONE;
> > +	char *pdevname = KBUILD_MODNAME "_leds";
> >  	int i;
> >  
> >  	platform_data.devmode = SIMATIC_IPC_DEVICE_NONE;
> > @@ -64,10 +65,12 @@ static int register_platform_devices(u32
> > station_id) }
> >  
> >  	if (ledmode != SIMATIC_IPC_DEVICE_NONE) {
> > +		if (ledmode == SIMATIC_IPC_DEVICE_127E)
> > +			pdevname = KBUILD_MODNAME "_leds_gpio";
> >  		platform_data.devmode = ledmode;
> >  		ipc_led_platform_device =
> >  			platform_device_register_data(NULL,
> > -				KBUILD_MODNAME "_leds",
> > PLATFORM_DEVID_NONE,
> > +				pdevname, PLATFORM_DEVID_NONE,
> >  				&platform_data,
> >  				sizeof(struct
> > simatic_ipc_platform)); if (IS_ERR(ipc_led_platform_device))  
> 

Re: [PATCH v6 00/12] platform/x86: introduce p2sb_bar() helper

[Hans de Goede]

Hi,

On 6/6/22 18:41, Andy Shevchenko wrote:
> There are a few users that would like to utilize P2SB mechanism of hiding
> and unhiding a device from the PCI configuration space.
> 
> Here is the series to consolidate p2sb handling code for existing users
> and to provide a generic way for new comer(s).
> 
> It also includes a patch to enable GPIO controllers on Apollo Lake
> when it's used with ABL bootloader w/o ACPI support.
> 
> The patch that brings the helper ("platform/x86/intel: Add Primary to
> Sideband (P2SB) bridge support") has a commit message that sheds a light
> on what the P2SB is and why this is needed.
> 
> I have tested this on Apollo Lake platform (I'm able to see SPI NOR and
> since we have an ACPI device for GPIO I do not see any attempts to recreate
> one).
> 
> The series is ready to be merged via MFD tree, but see below.
> 
> The series also includes updates for Simatic IPC drivers that partially
> tagged by respective maintainers (the main question is if Pavel is okay
> with the last three patches, since I believe Hans is okay with removing
> some code under PDx86). Hence the first 8 patches can be merged right
> away and the rest when Pavel does his review.
> 
> Changes in v6:
> - added tag to patch 5 (Lee)
> - incorporated Henning's series on top of v5

I've taken a quick look at the new patches and they look ok to me:

Acked-by: Hans de Goede <hdegoede@redhat.com>

for the entire series.

Regards,

Hans


> 
> Changes in v5:
> - rewritten patch 1 to use pci_scan_single_device() (Lukas, Bjorn)
> - rebased patch 2 on top of the new Intel SPI NOR codebase
> - fixed a potential bug and rewritten resource filling in patch 5 (Lee)
> - added many different tags in a few patches (Jean, Wolfram, Henning)
> 
> Changes in v4:
> - added tag to the entire series (Hans)
> - added tag to pin control patch (Mika)
> - dropped PCI core changes (PCI core doesn't want modifications to be made)
> - as a consequence of the above merged necessary bits into p2sb.c
> - added a check that p2sb is really hidden (Hans)
> - added EDAC patches (reviewed by maintainer internally)
> 
> Changes in v3:
> - resent with cover letter
> 
> Changes in v2:
> - added parentheses around bus in macros (Joe)
> - added tag (Jean)
> - fixed indentation and wrapping in the header (Christoph)
> - moved out of PCI realm to PDx86 as the best common denominator (Bjorn)
> - added a verbose commit message to explain P2SB thingy (Bjorn)
> - converted first parameter from pci_dev to pci_bus
> - made first two parameters (bus and devfn) optional (Henning, Lee)
> - added Intel pin control patch to the series (Henning, Mika)
> - fixed English style in the commit message of one of MFD patch (Lee)
> - added tags to my MFD LPC ICH patches (Lee)
> - used consistently (c) (Lee)
> - made indexing for MFD cell and resource arrays (Lee)
> - fixed the resource size in i801 (Jean)
> 
> Andy Shevchenko (6):
>   pinctrl: intel: Check against matching data instead of ACPI companion
>   mfd: lpc_ich: Factor out lpc_ich_enable_spi_write()
>   mfd: lpc_ich: Switch to generic p2sb_bar()
>   i2c: i801: convert to use common P2SB accessor
>   EDAC, pnd2: Use proper I/O accessors and address space annotation
>   EDAC, pnd2: convert to use common P2SB accessor
> 
> Henning Schild (4):
>   watchdog: simatic-ipc-wdt: convert to use P2SB accessor
>   leds: simatic-ipc-leds: convert to use P2SB accessor
>   platform/x86: simatic-ipc: drop custom P2SB bar code
>   leds: simatic-ipc-leds-gpio: add GPIO version of Siemens driver
> 
> Jonathan Yong (1):
>   platform/x86/intel: Add Primary to Sideband (P2SB) bridge support
> 
> Tan Jui Nee (1):
>   mfd: lpc_ich: Add support for pinctrl in non-ACPI system
> 
>  drivers/edac/Kconfig                          |   1 +
>  drivers/edac/pnd2_edac.c                      |  62 +++----
>  drivers/i2c/busses/Kconfig                    |   1 +
>  drivers/i2c/busses/i2c-i801.c                 |  39 +----
>  drivers/leds/simple/Kconfig                   |   6 +-
>  drivers/leds/simple/Makefile                  |   1 +
>  drivers/leds/simple/simatic-ipc-leds-gpio.c   | 105 ++++++++++++
>  drivers/leds/simple/simatic-ipc-leds.c        |  80 +--------
>  drivers/mfd/Kconfig                           |   1 +
>  drivers/mfd/lpc_ich.c                         | 161 ++++++++++++++----
>  drivers/pinctrl/intel/pinctrl-intel.c         |  14 +-
>  drivers/platform/x86/intel/Kconfig            |  12 ++
>  drivers/platform/x86/intel/Makefile           |   2 +
>  drivers/platform/x86/intel/p2sb.c             | 133 +++++++++++++++
>  drivers/platform/x86/simatic-ipc.c            |  43 +----
>  drivers/watchdog/Kconfig                      |   1 +
>  drivers/watchdog/simatic-ipc-wdt.c            |  15 +-
>  include/linux/platform_data/x86/p2sb.h        |  28 +++
>  .../platform_data/x86/simatic-ipc-base.h      |   2 -
>  19 files changed, 464 insertions(+), 243 deletions(-)
>  create mode 100644 drivers/leds/simple/simatic-ipc-leds-gpio.c
>  create mode 100644 drivers/platform/x86/intel/p2sb.c
>  create mode 100644 include/linux/platform_data/x86/p2sb.h
> 
> 
> base-commit: 40b58e42584bf5bd9230481dc8946f714fb387de

Re: [PATCH v6 00/12] platform/x86: introduce p2sb_bar() helper

[Lee Jones]

On Mon, 06 Jun 2022, Andy Shevchenko wrote:

> There are a few users that would like to utilize P2SB mechanism of hiding
> and unhiding a device from the PCI configuration space.
> 
> Here is the series to consolidate p2sb handling code for existing users
> and to provide a generic way for new comer(s).
> 
> It also includes a patch to enable GPIO controllers on Apollo Lake
> when it's used with ABL bootloader w/o ACPI support.
> 
> The patch that brings the helper ("platform/x86/intel: Add Primary to
> Sideband (P2SB) bridge support") has a commit message that sheds a light
> on what the P2SB is and why this is needed.
> 
> I have tested this on Apollo Lake platform (I'm able to see SPI NOR and
> since we have an ACPI device for GPIO I do not see any attempts to recreate
> one).
> 
> The series is ready to be merged via MFD tree, but see below.
> 
> The series also includes updates for Simatic IPC drivers that partially
> tagged by respective maintainers (the main question is if Pavel is okay
> with the last three patches, since I believe Hans is okay with removing
> some code under PDx86). Hence the first 8 patches can be merged right
> away and the rest when Pavel does his review.

Can we just wait for Pavel's review, then merge them all at once?

-- 
Lee Jones [李琼斯]
Principal Technical Lead - Developer Services
Linaro.org │ Open source software for Arm SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH v6 00/12] platform/x86: introduce p2sb_bar() helper

[Andy Shevchenko]

On Wed, Jun 8, 2022 at 9:42 AM Lee Jones <lee.jones@linaro.org> wrote:
>
> On Mon, 06 Jun 2022, Andy Shevchenko wrote:
>
> > There are a few users that would like to utilize P2SB mechanism of hiding
> > and unhiding a device from the PCI configuration space.
> >
> > Here is the series to consolidate p2sb handling code for existing users
> > and to provide a generic way for new comer(s).
> >
> > It also includes a patch to enable GPIO controllers on Apollo Lake
> > when it's used with ABL bootloader w/o ACPI support.
> >
> > The patch that brings the helper ("platform/x86/intel: Add Primary to
> > Sideband (P2SB) bridge support") has a commit message that sheds a light
> > on what the P2SB is and why this is needed.
> >
> > I have tested this on Apollo Lake platform (I'm able to see SPI NOR and
> > since we have an ACPI device for GPIO I do not see any attempts to recreate
> > one).
> >
> > The series is ready to be merged via MFD tree, but see below.
> >
> > The series also includes updates for Simatic IPC drivers that partially
> > tagged by respective maintainers (the main question is if Pavel is okay
> > with the last three patches, since I believe Hans is okay with removing
> > some code under PDx86). Hence the first 8 patches can be merged right
> > away and the rest when Pavel does his review.
>
> Can we just wait for Pavel's review, then merge them all at once?

Sure, it would be the best course of action.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v6 00/12] platform/x86: introduce p2sb_bar() helper

[Andy Shevchenko]

On Wed, Jun 08, 2022 at 12:50:44PM +0200, Andy Shevchenko wrote:
> On Wed, Jun 8, 2022 at 9:42 AM Lee Jones <lee.jones@linaro.org> wrote:
> >
> > On Mon, 06 Jun 2022, Andy Shevchenko wrote:
> >
> > > There are a few users that would like to utilize P2SB mechanism of hiding
> > > and unhiding a device from the PCI configuration space.
> > >
> > > Here is the series to consolidate p2sb handling code for existing users
> > > and to provide a generic way for new comer(s).
> > >
> > > It also includes a patch to enable GPIO controllers on Apollo Lake
> > > when it's used with ABL bootloader w/o ACPI support.
> > >
> > > The patch that brings the helper ("platform/x86/intel: Add Primary to
> > > Sideband (P2SB) bridge support") has a commit message that sheds a light
> > > on what the P2SB is and why this is needed.
> > >
> > > I have tested this on Apollo Lake platform (I'm able to see SPI NOR and
> > > since we have an ACPI device for GPIO I do not see any attempts to recreate
> > > one).
> > >
> > > The series is ready to be merged via MFD tree, but see below.
> > >
> > > The series also includes updates for Simatic IPC drivers that partially
> > > tagged by respective maintainers (the main question is if Pavel is okay
> > > with the last three patches, since I believe Hans is okay with removing
> > > some code under PDx86). Hence the first 8 patches can be merged right
> > > away and the rest when Pavel does his review.
> >
> > Can we just wait for Pavel's review, then merge them all at once?
> 
> Sure, it would be the best course of action.

Pavel, do you have a chance to review the patches (last three) that touch
LED drivers? What would be your verdict?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v6 00/12] platform/x86: introduce p2sb_bar() helper

[Andy Shevchenko]

+Cc: Rafael

On Tue, Jun 21, 2022 at 02:58:16PM +0300, Andy Shevchenko wrote:
> On Wed, Jun 08, 2022 at 12:50:44PM +0200, Andy Shevchenko wrote:
> > On Wed, Jun 8, 2022 at 9:42 AM Lee Jones <lee.jones@linaro.org> wrote:
> > > On Mon, 06 Jun 2022, Andy Shevchenko wrote:
> > >
> > > > There are a few users that would like to utilize P2SB mechanism of hiding
> > > > and unhiding a device from the PCI configuration space.
> > > >
> > > > Here is the series to consolidate p2sb handling code for existing users
> > > > and to provide a generic way for new comer(s).
> > > >
> > > > It also includes a patch to enable GPIO controllers on Apollo Lake
> > > > when it's used with ABL bootloader w/o ACPI support.
> > > >
> > > > The patch that brings the helper ("platform/x86/intel: Add Primary to
> > > > Sideband (P2SB) bridge support") has a commit message that sheds a light
> > > > on what the P2SB is and why this is needed.
> > > >
> > > > I have tested this on Apollo Lake platform (I'm able to see SPI NOR and
> > > > since we have an ACPI device for GPIO I do not see any attempts to recreate
> > > > one).
> > > >
> > > > The series is ready to be merged via MFD tree, but see below.
> > > >
> > > > The series also includes updates for Simatic IPC drivers that partially
> > > > tagged by respective maintainers (the main question is if Pavel is okay
> > > > with the last three patches, since I believe Hans is okay with removing
> > > > some code under PDx86). Hence the first 8 patches can be merged right
> > > > away and the rest when Pavel does his review.
> > >
> > > Can we just wait for Pavel's review, then merge them all at once?
> > 
> > Sure, it would be the best course of action.
> 
> Pavel, do you have a chance to review the patches (last three) that touch
> LED drivers? What would be your verdict?

Lee, Rafael,

It seems quite hard to get Pavel's attention to this series [1]. It's already
passed more than 3 weeks for any sign of review of three top patches of the
series that touched LED subsystem. The entire series has all necessary tags,
but for LED changes.

Note, that the top of this series is not done by me and was sent for
preliminary review much earlier [2], altogether it makes months of no response from
the maintainer.

The nature of patches is pretty simple and doesn't touch any of other than Simatic LED
drivers nor LED core. Moreover, it was written by Siemens, who produces the H/W in
question and very well tested as a separate change and as part of the series.

I think to move forward we may ask Rafael to review it on behalf of good maintainer
and with his approval apply entire series.

Thoughts?

[1]: https://lore.kernel.org/all/20220606164138.66535-1-andriy.shevchenko@linux.intel.com/
[2]: https://lore.kernel.org/linux-leds/20220513083652.974-1-henning.schild@siemens.com/

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v6 00/12] platform/x86: introduce p2sb_bar() helper

[Henning Schild]

Am Wed, 29 Jun 2022 19:39:58 +0300
schrieb Andy Shevchenko <andriy.shevchenko@linux.intel.com>:

> +Cc: Rafael
> 
> On Tue, Jun 21, 2022 at 02:58:16PM +0300, Andy Shevchenko wrote:
> > On Wed, Jun 08, 2022 at 12:50:44PM +0200, Andy Shevchenko wrote:  
> > > On Wed, Jun 8, 2022 at 9:42 AM Lee Jones <lee.jones@linaro.org>
> > > wrote:  
> > > > On Mon, 06 Jun 2022, Andy Shevchenko wrote:
> > > >  
> > > > > There are a few users that would like to utilize P2SB
> > > > > mechanism of hiding and unhiding a device from the PCI
> > > > > configuration space.
> > > > >
> > > > > Here is the series to consolidate p2sb handling code for
> > > > > existing users and to provide a generic way for new comer(s).
> > > > >
> > > > > It also includes a patch to enable GPIO controllers on Apollo
> > > > > Lake when it's used with ABL bootloader w/o ACPI support.
> > > > >
> > > > > The patch that brings the helper ("platform/x86/intel: Add
> > > > > Primary to Sideband (P2SB) bridge support") has a commit
> > > > > message that sheds a light on what the P2SB is and why this
> > > > > is needed.
> > > > >
> > > > > I have tested this on Apollo Lake platform (I'm able to see
> > > > > SPI NOR and since we have an ACPI device for GPIO I do not
> > > > > see any attempts to recreate one).
> > > > >
> > > > > The series is ready to be merged via MFD tree, but see below.
> > > > >
> > > > > The series also includes updates for Simatic IPC drivers that
> > > > > partially tagged by respective maintainers (the main question
> > > > > is if Pavel is okay with the last three patches, since I
> > > > > believe Hans is okay with removing some code under PDx86).
> > > > > Hence the first 8 patches can be merged right away and the
> > > > > rest when Pavel does his review.  
> > > >
> > > > Can we just wait for Pavel's review, then merge them all at
> > > > once?  
> > > 
> > > Sure, it would be the best course of action.  
> > 
> > Pavel, do you have a chance to review the patches (last three) that
> > touch LED drivers? What would be your verdict?  
> 
> Lee, Rafael,
> 
> It seems quite hard to get Pavel's attention to this series [1]. It's
> already passed more than 3 weeks for any sign of review of three top
> patches of the series that touched LED subsystem. The entire series
> has all necessary tags, but for LED changes.
> 
> Note, that the top of this series is not done by me and was sent for
> preliminary review much earlier [2], altogether it makes months of no
> response from the maintainer.
> 
> The nature of patches is pretty simple and doesn't touch any of other
> than Simatic LED drivers nor LED core. Moreover, it was written by
> Siemens, who produces the H/W in question and very well tested as a
> separate change and as part of the series.

The code has been reviewed and is in fact pretty simple. The only
questionable but pragmatic change that might catch the attention of a
pedantic reviewer is that i did put the gpio implementation of the
driver under the same/existing kernel config switch.

> I think to move forward we may ask Rafael to review it on behalf of
> good maintainer and with his approval apply entire series.
> 
> Thoughts?

Thanks for pushing this Andy. I was wondering how and when that story
would continue. Technically these changes should really go in one badge
or we need to find a way to separate them somehow. I would try to go
that extra mile to get out of your way. But i am kind of afraid such an
effort might also end up touching the same files and block us at the
same maintainer.

Did anyone check whether Pavel was active at all in those last months
and maybe other patches waiting for review? Hope he is fine and active
and just somehow forgot/overlooked/ignored this one.

Henning

> [1]:
> https://lore.kernel.org/all/20220606164138.66535-1-andriy.shevchenko@linux.intel.com/
> [2]:
> https://lore.kernel.org/linux-leds/20220513083652.974-1-henning.schild@siemens.com/
> 

Re: [PATCH v6 00/12] platform/x86: introduce p2sb_bar() helper

[Andy Shevchenko]

On Wed, Jun 29, 2022 at 07:14:06PM +0200, Henning Schild wrote:
> Am Wed, 29 Jun 2022 19:39:58 +0300
> schrieb Andy Shevchenko <andriy.shevchenko@linux.intel.com>:
> 
> > +Cc: Rafael
> > 
> > On Tue, Jun 21, 2022 at 02:58:16PM +0300, Andy Shevchenko wrote:
> > > On Wed, Jun 08, 2022 at 12:50:44PM +0200, Andy Shevchenko wrote:  
> > > > On Wed, Jun 8, 2022 at 9:42 AM Lee Jones <lee.jones@linaro.org>
> > > > wrote:  
> > > > > On Mon, 06 Jun 2022, Andy Shevchenko wrote:
> > > > >  
> > > > > > There are a few users that would like to utilize P2SB
> > > > > > mechanism of hiding and unhiding a device from the PCI
> > > > > > configuration space.
> > > > > >
> > > > > > Here is the series to consolidate p2sb handling code for
> > > > > > existing users and to provide a generic way for new comer(s).
> > > > > >
> > > > > > It also includes a patch to enable GPIO controllers on Apollo
> > > > > > Lake when it's used with ABL bootloader w/o ACPI support.
> > > > > >
> > > > > > The patch that brings the helper ("platform/x86/intel: Add
> > > > > > Primary to Sideband (P2SB) bridge support") has a commit
> > > > > > message that sheds a light on what the P2SB is and why this
> > > > > > is needed.
> > > > > >
> > > > > > I have tested this on Apollo Lake platform (I'm able to see
> > > > > > SPI NOR and since we have an ACPI device for GPIO I do not
> > > > > > see any attempts to recreate one).
> > > > > >
> > > > > > The series is ready to be merged via MFD tree, but see below.
> > > > > >
> > > > > > The series also includes updates for Simatic IPC drivers that
> > > > > > partially tagged by respective maintainers (the main question
> > > > > > is if Pavel is okay with the last three patches, since I
> > > > > > believe Hans is okay with removing some code under PDx86).
> > > > > > Hence the first 8 patches can be merged right away and the
> > > > > > rest when Pavel does his review.  
> > > > >
> > > > > Can we just wait for Pavel's review, then merge them all at
> > > > > once?  
> > > > 
> > > > Sure, it would be the best course of action.  
> > > 
> > > Pavel, do you have a chance to review the patches (last three) that
> > > touch LED drivers? What would be your verdict?  
> > 
> > Lee, Rafael,
> > 
> > It seems quite hard to get Pavel's attention to this series [1]. It's
> > already passed more than 3 weeks for any sign of review of three top
> > patches of the series that touched LED subsystem. The entire series
> > has all necessary tags, but for LED changes.
> > 
> > Note, that the top of this series is not done by me and was sent for
> > preliminary review much earlier [2], altogether it makes months of no
> > response from the maintainer.
> > 
> > The nature of patches is pretty simple and doesn't touch any of other
> > than Simatic LED drivers nor LED core. Moreover, it was written by
> > Siemens, who produces the H/W in question and very well tested as a
> > separate change and as part of the series.
> 
> The code has been reviewed and is in fact pretty simple. The only
> questionable but pragmatic change that might catch the attention of a
> pedantic reviewer is that i did put the gpio implementation of the
> driver under the same/existing kernel config switch.
> 
> > I think to move forward we may ask Rafael to review it on behalf of
> > good maintainer and with his approval apply entire series.
> > 
> > Thoughts?
> 
> Thanks for pushing this Andy. I was wondering how and when that story
> would continue. Technically these changes should really go in one badge
> or we need to find a way to separate them somehow. I would try to go
> that extra mile to get out of your way. But i am kind of afraid such an
> effort might also end up touching the same files and block us at the
> same maintainer.
> 
> Did anyone check whether Pavel was active at all in those last months
> and maybe other patches waiting for review? Hope he is fine and active
> and just somehow forgot/overlooked/ignored this one.

I have send a private mail to Pavel and have got no response.
Can we move this forward, let's say, by applying first 8 patches?

> > [1]:
> > https://lore.kernel.org/all/20220606164138.66535-1-andriy.shevchenko@linux.intel.com/
> > [2]:
> > https://lore.kernel.org/linux-leds/20220513083652.974-1-henning.schild@siemens.com/
> 

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v6 00/12] platform/x86: introduce p2sb_bar() helper

[Rafael J. Wysocki]

On Wed, Jul 13, 2022 at 6:40 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> On Wed, Jun 29, 2022 at 07:14:06PM +0200, Henning Schild wrote:
> > Am Wed, 29 Jun 2022 19:39:58 +0300
> > schrieb Andy Shevchenko <andriy.shevchenko@linux.intel.com>:
> >
> > > +Cc: Rafael
> > >
> > > On Tue, Jun 21, 2022 at 02:58:16PM +0300, Andy Shevchenko wrote:
> > > > On Wed, Jun 08, 2022 at 12:50:44PM +0200, Andy Shevchenko wrote:
> > > > > On Wed, Jun 8, 2022 at 9:42 AM Lee Jones <lee.jones@linaro.org>
> > > > > wrote:
> > > > > > On Mon, 06 Jun 2022, Andy Shevchenko wrote:
> > > > > >
> > > > > > > There are a few users that would like to utilize P2SB
> > > > > > > mechanism of hiding and unhiding a device from the PCI
> > > > > > > configuration space.
> > > > > > >
> > > > > > > Here is the series to consolidate p2sb handling code for
> > > > > > > existing users and to provide a generic way for new comer(s).
> > > > > > >
> > > > > > > It also includes a patch to enable GPIO controllers on Apollo
> > > > > > > Lake when it's used with ABL bootloader w/o ACPI support.
> > > > > > >
> > > > > > > The patch that brings the helper ("platform/x86/intel: Add
> > > > > > > Primary to Sideband (P2SB) bridge support") has a commit
> > > > > > > message that sheds a light on what the P2SB is and why this
> > > > > > > is needed.
> > > > > > >
> > > > > > > I have tested this on Apollo Lake platform (I'm able to see
> > > > > > > SPI NOR and since we have an ACPI device for GPIO I do not
> > > > > > > see any attempts to recreate one).
> > > > > > >
> > > > > > > The series is ready to be merged via MFD tree, but see below.
> > > > > > >
> > > > > > > The series also includes updates for Simatic IPC drivers that
> > > > > > > partially tagged by respective maintainers (the main question
> > > > > > > is if Pavel is okay with the last three patches, since I
> > > > > > > believe Hans is okay with removing some code under PDx86).
> > > > > > > Hence the first 8 patches can be merged right away and the
> > > > > > > rest when Pavel does his review.
> > > > > >
> > > > > > Can we just wait for Pavel's review, then merge them all at
> > > > > > once?
> > > > >
> > > > > Sure, it would be the best course of action.
> > > >
> > > > Pavel, do you have a chance to review the patches (last three) that
> > > > touch LED drivers? What would be your verdict?
> > >
> > > Lee, Rafael,
> > >
> > > It seems quite hard to get Pavel's attention to this series [1]. It's
> > > already passed more than 3 weeks for any sign of review of three top
> > > patches of the series that touched LED subsystem. The entire series
> > > has all necessary tags, but for LED changes.
> > >
> > > Note, that the top of this series is not done by me and was sent for
> > > preliminary review much earlier [2], altogether it makes months of no
> > > response from the maintainer.
> > >
> > > The nature of patches is pretty simple and doesn't touch any of other
> > > than Simatic LED drivers nor LED core. Moreover, it was written by
> > > Siemens, who produces the H/W in question and very well tested as a
> > > separate change and as part of the series.
> >
> > The code has been reviewed and is in fact pretty simple. The only
> > questionable but pragmatic change that might catch the attention of a
> > pedantic reviewer is that i did put the gpio implementation of the
> > driver under the same/existing kernel config switch.
> >
> > > I think to move forward we may ask Rafael to review it on behalf of
> > > good maintainer and with his approval apply entire series.
> > >
> > > Thoughts?
> >
> > Thanks for pushing this Andy. I was wondering how and when that story
> > would continue. Technically these changes should really go in one badge
> > or we need to find a way to separate them somehow. I would try to go
> > that extra mile to get out of your way. But i am kind of afraid such an
> > effort might also end up touching the same files and block us at the
> > same maintainer.
> >
> > Did anyone check whether Pavel was active at all in those last months
> > and maybe other patches waiting for review? Hope he is fine and active
> > and just somehow forgot/overlooked/ignored this one.
>
> I have send a private mail to Pavel and have got no response.
> Can we move this forward, let's say, by applying first 8 patches?

IMV, everything up to and including patc [11/12] in the series should
be good to go.  There are ACKs from the relevant maintainers on that
material and I'm not sure what Pavel can add to that TBH.

> > > [1]:
> > > https://lore.kernel.org/all/20220606164138.66535-1-andriy.shevchenko@linux.intel.com/
> > > [2]:
> > > https://lore.kernel.org/linux-leds/20220513083652.974-1-henning.schild@siemens.com/
> >

Re: [PATCH v6 00/12] platform/x86: introduce p2sb_bar() helper

[Henning Schild]

Am Wed, 13 Jul 2022 19:40:23 +0300
schrieb Andy Shevchenko <andriy.shevchenko@linux.intel.com>:

> On Wed, Jun 29, 2022 at 07:14:06PM +0200, Henning Schild wrote:
> > Am Wed, 29 Jun 2022 19:39:58 +0300
> > schrieb Andy Shevchenko <andriy.shevchenko@linux.intel.com>:
> >   
> > > +Cc: Rafael
> > > 
> > > On Tue, Jun 21, 2022 at 02:58:16PM +0300, Andy Shevchenko wrote:  
> > > > On Wed, Jun 08, 2022 at 12:50:44PM +0200, Andy Shevchenko
> > > > wrote:    
> > > > > On Wed, Jun 8, 2022 at 9:42 AM Lee Jones
> > > > > <lee.jones@linaro.org> wrote:    
> > > > > > On Mon, 06 Jun 2022, Andy Shevchenko wrote:
> > > > > >    
> > > > > > > There are a few users that would like to utilize P2SB
> > > > > > > mechanism of hiding and unhiding a device from the PCI
> > > > > > > configuration space.
> > > > > > >
> > > > > > > Here is the series to consolidate p2sb handling code for
> > > > > > > existing users and to provide a generic way for new
> > > > > > > comer(s).
> > > > > > >
> > > > > > > It also includes a patch to enable GPIO controllers on
> > > > > > > Apollo Lake when it's used with ABL bootloader w/o ACPI
> > > > > > > support.
> > > > > > >
> > > > > > > The patch that brings the helper ("platform/x86/intel: Add
> > > > > > > Primary to Sideband (P2SB) bridge support") has a commit
> > > > > > > message that sheds a light on what the P2SB is and why
> > > > > > > this is needed.
> > > > > > >
> > > > > > > I have tested this on Apollo Lake platform (I'm able to
> > > > > > > see SPI NOR and since we have an ACPI device for GPIO I
> > > > > > > do not see any attempts to recreate one).
> > > > > > >
> > > > > > > The series is ready to be merged via MFD tree, but see
> > > > > > > below.
> > > > > > >
> > > > > > > The series also includes updates for Simatic IPC drivers
> > > > > > > that partially tagged by respective maintainers (the main
> > > > > > > question is if Pavel is okay with the last three patches,
> > > > > > > since I believe Hans is okay with removing some code
> > > > > > > under PDx86). Hence the first 8 patches can be merged
> > > > > > > right away and the rest when Pavel does his review.    
> > > > > >
> > > > > > Can we just wait for Pavel's review, then merge them all at
> > > > > > once?    
> > > > > 
> > > > > Sure, it would be the best course of action.    
> > > > 
> > > > Pavel, do you have a chance to review the patches (last three)
> > > > that touch LED drivers? What would be your verdict?    
> > > 
> > > Lee, Rafael,
> > > 
> > > It seems quite hard to get Pavel's attention to this series [1].
> > > It's already passed more than 3 weeks for any sign of review of
> > > three top patches of the series that touched LED subsystem. The
> > > entire series has all necessary tags, but for LED changes.
> > > 
> > > Note, that the top of this series is not done by me and was sent
> > > for preliminary review much earlier [2], altogether it makes
> > > months of no response from the maintainer.
> > > 
> > > The nature of patches is pretty simple and doesn't touch any of
> > > other than Simatic LED drivers nor LED core. Moreover, it was
> > > written by Siemens, who produces the H/W in question and very
> > > well tested as a separate change and as part of the series.  
> > 
> > The code has been reviewed and is in fact pretty simple. The only
> > questionable but pragmatic change that might catch the attention of
> > a pedantic reviewer is that i did put the gpio implementation of the
> > driver under the same/existing kernel config switch.
> >   
> > > I think to move forward we may ask Rafael to review it on behalf
> > > of good maintainer and with his approval apply entire series.
> > > 
> > > Thoughts?  
> > 
> > Thanks for pushing this Andy. I was wondering how and when that
> > story would continue. Technically these changes should really go in
> > one badge or we need to find a way to separate them somehow. I
> > would try to go that extra mile to get out of your way. But i am
> > kind of afraid such an effort might also end up touching the same
> > files and block us at the same maintainer.
> > 
> > Did anyone check whether Pavel was active at all in those last
> > months and maybe other patches waiting for review? Hope he is fine
> > and active and just somehow forgot/overlooked/ignored this one.  
> 
> I have send a private mail to Pavel and have got no response.
> Can we move this forward, let's say, by applying first 8 patches?

I am sorry that situation is now coming. Both simatic-ipc and that
appollo lake pinctrl driver compete for the same device memory. That
conflict was known and we agreed on sorting it out together somehow.
Not applying my patches could leave my LED drivers simply not working
any longer, or worse ... them making the apollolake platform stuff act
up somehow weird with unexpected EBUSY.

The series can not be split, or we have to write additional code to
properly deal with the conflict. I could envision my LED drivers still
accessing raw memory and ignoring EBUSY (very hacky! ... and touching
"we need Pavel code")

Another way could maybe be. Do the whole P2SB but do not make
apollolake pinctrl come up without ACPI. Somewhere in patches 1-8 there
is code which makes the pinctrl stuff come up for certain CPUs without
ACPI. It is really only some out of many CPUs which have pinctrl, and i
am not sure i remember what that has to do with the P2SB helpers as
such. The helpers are a refactoring, while the "bring up apollolake
pinctrl at all times" is a functional change ... now causing conflict.

And maybe there is a way/process to escalate to another maintainer.
Does anyone even know what is going on with Pavel? 

Henning

> > > [1]:
> > > https://lore.kernel.org/all/20220606164138.66535-1-andriy.shevchenko@linux.intel.com/
> > > [2]:
> > > https://lore.kernel.org/linux-leds/20220513083652.974-1-henning.schild@siemens.com/
> > >  
> >   
> 

Re: [PATCH v6 00/12] platform/x86: introduce p2sb_bar() helper

[Lee Jones]

On Wed, 13 Jul 2022, Henning Schild wrote:

> Am Wed, 13 Jul 2022 19:40:23 +0300
> schrieb Andy Shevchenko <andriy.shevchenko@linux.intel.com>:
> 
> > On Wed, Jun 29, 2022 at 07:14:06PM +0200, Henning Schild wrote:
> > > Am Wed, 29 Jun 2022 19:39:58 +0300
> > > schrieb Andy Shevchenko <andriy.shevchenko@linux.intel.com>:
> > >   
> > > > +Cc: Rafael
> > > > 
> > > > On Tue, Jun 21, 2022 at 02:58:16PM +0300, Andy Shevchenko wrote:  
> > > > > On Wed, Jun 08, 2022 at 12:50:44PM +0200, Andy Shevchenko
> > > > > wrote:    
> > > > > > On Wed, Jun 8, 2022 at 9:42 AM Lee Jones
> > > > > > <lee.jones@linaro.org> wrote:    
> > > > > > > On Mon, 06 Jun 2022, Andy Shevchenko wrote:
> > > > > > >    
> > > > > > > > There are a few users that would like to utilize P2SB
> > > > > > > > mechanism of hiding and unhiding a device from the PCI
> > > > > > > > configuration space.
> > > > > > > >
> > > > > > > > Here is the series to consolidate p2sb handling code for
> > > > > > > > existing users and to provide a generic way for new
> > > > > > > > comer(s).
> > > > > > > >
> > > > > > > > It also includes a patch to enable GPIO controllers on
> > > > > > > > Apollo Lake when it's used with ABL bootloader w/o ACPI
> > > > > > > > support.
> > > > > > > >
> > > > > > > > The patch that brings the helper ("platform/x86/intel: Add
> > > > > > > > Primary to Sideband (P2SB) bridge support") has a commit
> > > > > > > > message that sheds a light on what the P2SB is and why
> > > > > > > > this is needed.
> > > > > > > >
> > > > > > > > I have tested this on Apollo Lake platform (I'm able to
> > > > > > > > see SPI NOR and since we have an ACPI device for GPIO I
> > > > > > > > do not see any attempts to recreate one).
> > > > > > > >
> > > > > > > > The series is ready to be merged via MFD tree, but see
> > > > > > > > below.
> > > > > > > >
> > > > > > > > The series also includes updates for Simatic IPC drivers
> > > > > > > > that partially tagged by respective maintainers (the main
> > > > > > > > question is if Pavel is okay with the last three patches,
> > > > > > > > since I believe Hans is okay with removing some code
> > > > > > > > under PDx86). Hence the first 8 patches can be merged
> > > > > > > > right away and the rest when Pavel does his review.    
> > > > > > >
> > > > > > > Can we just wait for Pavel's review, then merge them all at
> > > > > > > once?    
> > > > > > 
> > > > > > Sure, it would be the best course of action.    
> > > > > 
> > > > > Pavel, do you have a chance to review the patches (last three)
> > > > > that touch LED drivers? What would be your verdict?    
> > > > 
> > > > Lee, Rafael,
> > > > 
> > > > It seems quite hard to get Pavel's attention to this series [1].
> > > > It's already passed more than 3 weeks for any sign of review of
> > > > three top patches of the series that touched LED subsystem. The
> > > > entire series has all necessary tags, but for LED changes.
> > > > 
> > > > Note, that the top of this series is not done by me and was sent
> > > > for preliminary review much earlier [2], altogether it makes
> > > > months of no response from the maintainer.
> > > > 
> > > > The nature of patches is pretty simple and doesn't touch any of
> > > > other than Simatic LED drivers nor LED core. Moreover, it was
> > > > written by Siemens, who produces the H/W in question and very
> > > > well tested as a separate change and as part of the series.  
> > > 
> > > The code has been reviewed and is in fact pretty simple. The only
> > > questionable but pragmatic change that might catch the attention of
> > > a pedantic reviewer is that i did put the gpio implementation of the
> > > driver under the same/existing kernel config switch.
> > >   
> > > > I think to move forward we may ask Rafael to review it on behalf
> > > > of good maintainer and with his approval apply entire series.
> > > > 
> > > > Thoughts?  
> > > 
> > > Thanks for pushing this Andy. I was wondering how and when that
> > > story would continue. Technically these changes should really go in
> > > one badge or we need to find a way to separate them somehow. I
> > > would try to go that extra mile to get out of your way. But i am
> > > kind of afraid such an effort might also end up touching the same
> > > files and block us at the same maintainer.
> > > 
> > > Did anyone check whether Pavel was active at all in those last
> > > months and maybe other patches waiting for review? Hope he is fine
> > > and active and just somehow forgot/overlooked/ignored this one.  
> > 
> > I have send a private mail to Pavel and have got no response.
> > Can we move this forward, let's say, by applying first 8 patches?
> 
> I am sorry that situation is now coming. Both simatic-ipc and that
> appollo lake pinctrl driver compete for the same device memory. That
> conflict was known and we agreed on sorting it out together somehow.
> Not applying my patches could leave my LED drivers simply not working
> any longer, or worse ... them making the apollolake platform stuff act
> up somehow weird with unexpected EBUSY.
> 
> The series can not be split, or we have to write additional code to
> properly deal with the conflict. I could envision my LED drivers still
> accessing raw memory and ignoring EBUSY (very hacky! ... and touching
> "we need Pavel code")
> 
> Another way could maybe be. Do the whole P2SB but do not make
> apollolake pinctrl come up without ACPI. Somewhere in patches 1-8 there
> is code which makes the pinctrl stuff come up for certain CPUs without
> ACPI. It is really only some out of many CPUs which have pinctrl, and i
> am not sure i remember what that has to do with the P2SB helpers as
> such. The helpers are a refactoring, while the "bring up apollolake
> pinctrl at all times" is a functional change ... now causing conflict.
> 
> And maybe there is a way/process to escalate to another maintainer.
> Does anyone even know what is going on with Pavel? 

I'll take the hit.  He had his chance.

I'm happy to move forward with Andy's review.

(Side note: Seeing as Pavel hasn't been seen for 2 months, I'll also
 follow-up on  the LED ML to offer to become temporary maintainer for a
 bit)

-- 
Lee Jones [李琼斯]
Principal Technical Lead - Developer Services
Linaro.org │ Open source software for Arm SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH v6 00/12] platform/x86: introduce p2sb_bar() helper

[Andy Shevchenko]

On Thu, Jul 14, 2022 at 10:37:19AM +0100, Lee Jones wrote:
> On Wed, 13 Jul 2022, Henning Schild wrote:
> > Am Wed, 13 Jul 2022 19:40:23 +0300
> > schrieb Andy Shevchenko <andriy.shevchenko@linux.intel.com>:

...

> > And maybe there is a way/process to escalate to another maintainer.
> > Does anyone even know what is going on with Pavel? 
> 
> I'll take the hit.  He had his chance.
> 
> I'm happy to move forward with Andy's review.

Thank you, Lee, much appreciated!
The patches (9..12) have my SoB, I think it should be enough, but if you thinks
they need my Rb tag, I can reply to them with it.

> (Side note: Seeing as Pavel hasn't been seen for 2 months, I'll also
>  follow-up on  the LED ML to offer to become temporary maintainer for a
>  bit)

This is good news as well, because I noticed there are a few series there stuck
as well.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v6 00/12] platform/x86: introduce p2sb_bar() helper

[Lee Jones]

On Thu, 14 Jul 2022, Andy Shevchenko wrote:

> On Thu, Jul 14, 2022 at 10:37:19AM +0100, Lee Jones wrote:
> > On Wed, 13 Jul 2022, Henning Schild wrote:
> > > Am Wed, 13 Jul 2022 19:40:23 +0300
> > > schrieb Andy Shevchenko <andriy.shevchenko@linux.intel.com>:
> 
> ...
> 
> > > And maybe there is a way/process to escalate to another maintainer.
> > > Does anyone even know what is going on with Pavel? 
> > 
> > I'll take the hit.  He had his chance.
> > 
> > I'm happy to move forward with Andy's review.
> 
> Thank you, Lee, much appreciated!
> The patches (9..12) have my SoB, I think it should be enough, but if you thinks
> they need my Rb tag, I can reply to them with it.

That's okay.  I've applied them and they are currently in soak.

> > (Side note: Seeing as Pavel hasn't been seen for 2 months, I'll also
> >  follow-up on  the LED ML to offer to become temporary maintainer for a
> >  bit)
> 
> This is good news as well, because I noticed there are a few series there stuck
> as well.

Feel free to reply to object or in support:

https://lore.kernel.org/all/Ys%2Fkruf8DE4ISo8M@google.com/

-- 
Lee Jones [李琼斯]
Principal Technical Lead - Developer Services
Linaro.org │ Open source software for Arm SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH v6 00/12] platform/x86: introduce p2sb_bar() helper

[Henning Schild]

Am Thu, 14 Jul 2022 11:34:31 +0100
schrieb Lee Jones <lee.jones@linaro.org>:

> On Thu, 14 Jul 2022, Andy Shevchenko wrote:
> 
> > On Thu, Jul 14, 2022 at 10:37:19AM +0100, Lee Jones wrote:  
> > > On Wed, 13 Jul 2022, Henning Schild wrote:  
> > > > Am Wed, 13 Jul 2022 19:40:23 +0300
> > > > schrieb Andy Shevchenko <andriy.shevchenko@linux.intel.com>:  
> > 
> > ...
> >   
> > > > And maybe there is a way/process to escalate to another
> > > > maintainer. Does anyone even know what is going on with Pavel?
> > > >  
> > > 
> > > I'll take the hit.  He had his chance.
> > > 
> > > I'm happy to move forward with Andy's review.  
> > 
> > Thank you, Lee, much appreciated!
> > The patches (9..12) have my SoB, I think it should be enough, but
> > if you thinks they need my Rb tag, I can reply to them with it.  
> 
> That's okay.  I've applied them and they are currently in soak.

Very good news and thanks so much!

Henning

> > > (Side note: Seeing as Pavel hasn't been seen for 2 months, I'll
> > > also follow-up on  the LED ML to offer to become temporary
> > > maintainer for a bit)  
> > 
> > This is good news as well, because I noticed there are a few series
> > there stuck as well.  
> 
> Feel free to reply to object or in support:
> 
> https://lore.kernel.org/all/Ys%2Fkruf8DE4ISo8M@google.com/
> 

Re: [PATCH v6 00/12] platform/x86: introduce p2sb_bar() helper

[Pavel Machek]

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

On Wed 2022-06-08 08:42:40, Lee Jones wrote:
> On Mon, 06 Jun 2022, Andy Shevchenko wrote:
> 
> > There are a few users that would like to utilize P2SB mechanism of hiding
> > and unhiding a device from the PCI configuration space.
> > 
> > Here is the series to consolidate p2sb handling code for existing users
> > and to provide a generic way for new comer(s).
> > 
> > It also includes a patch to enable GPIO controllers on Apollo Lake
> > when it's used with ABL bootloader w/o ACPI support.
> > 
> > The patch that brings the helper ("platform/x86/intel: Add Primary to
> > Sideband (P2SB) bridge support") has a commit message that sheds a light
> > on what the P2SB is and why this is needed.
> > 
> > I have tested this on Apollo Lake platform (I'm able to see SPI NOR and
> > since we have an ACPI device for GPIO I do not see any attempts to recreate
> > one).
> > 
> > The series is ready to be merged via MFD tree, but see below.
> > 
> > The series also includes updates for Simatic IPC drivers that partially
> > tagged by respective maintainers (the main question is if Pavel is okay
> > with the last three patches, since I believe Hans is okay with removing
> > some code under PDx86). Hence the first 8 patches can be merged right
> > away and the rest when Pavel does his review.
> 
> Can we just wait for Pavel's review, then merge them all at once?

10,12: Acked-by: Pavel Machek <pavel@ucw.cz>
								Pavel
-- 
People of Russia, stop Putin before his war on Ukraine escalates.

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

Re: [PATCH v6 00/12] platform/x86: introduce p2sb_bar() helper

[Lee Jones]

On Thu, 14 Jul 2022, Pavel Machek wrote:

> On Wed 2022-06-08 08:42:40, Lee Jones wrote:
> > On Mon, 06 Jun 2022, Andy Shevchenko wrote:
> > 
> > > There are a few users that would like to utilize P2SB mechanism of hiding
> > > and unhiding a device from the PCI configuration space.
> > > 
> > > Here is the series to consolidate p2sb handling code for existing users
> > > and to provide a generic way for new comer(s).
> > > 
> > > It also includes a patch to enable GPIO controllers on Apollo Lake
> > > when it's used with ABL bootloader w/o ACPI support.
> > > 
> > > The patch that brings the helper ("platform/x86/intel: Add Primary to
> > > Sideband (P2SB) bridge support") has a commit message that sheds a light
> > > on what the P2SB is and why this is needed.
> > > 
> > > I have tested this on Apollo Lake platform (I'm able to see SPI NOR and
> > > since we have an ACPI device for GPIO I do not see any attempts to recreate
> > > one).
> > > 
> > > The series is ready to be merged via MFD tree, but see below.
> > > 
> > > The series also includes updates for Simatic IPC drivers that partially
> > > tagged by respective maintainers (the main question is if Pavel is okay
> > > with the last three patches, since I believe Hans is okay with removing
> > > some code under PDx86). Hence the first 8 patches can be merged right
> > > away and the rest when Pavel does his review.
> > 
> > Can we just wait for Pavel's review, then merge them all at once?
> 
> 10,12: Acked-by: Pavel Machek <pavel@ucw.cz>

Thanks Pavel.  I'll get that added.

-- 
Lee Jones [李琼斯]
Principal Technical Lead - Developer Services
Linaro.org │ Open source software for Arm SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH v6 00/12] platform/x86: introduce p2sb_bar() helper

[Pavel Machek]

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

Hi!

> > > Can we just wait for Pavel's review, then merge them all at once?
> > 
> > 10,12: Acked-by: Pavel Machek <pavel@ucw.cz>
> 
> Thanks Pavel.  I'll get that added.

Thank you, sorry for the delays.

Best regards,
							Pavel

-- 
People of Russia, stop Putin before his war on Ukraine escalates.

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

Re: [PATCH v6 00/12] platform/x86: introduce p2sb_bar() helper

[Andy Shevchenko]

On Mon, Jun 06, 2022 at 07:41:26PM +0300, Andy Shevchenko wrote:
> There are a few users that would like to utilize P2SB mechanism of hiding
> and unhiding a device from the PCI configuration space.
> 
> Here is the series to consolidate p2sb handling code for existing users
> and to provide a generic way for new comer(s).
> 
> It also includes a patch to enable GPIO controllers on Apollo Lake
> when it's used with ABL bootloader w/o ACPI support.
> 
> The patch that brings the helper ("platform/x86/intel: Add Primary to
> Sideband (P2SB) bridge support") has a commit message that sheds a light
> on what the P2SB is and why this is needed.
> 
> I have tested this on Apollo Lake platform (I'm able to see SPI NOR and
> since we have an ACPI device for GPIO I do not see any attempts to recreate
> one).
> 
> The series is ready to be merged via MFD tree, but see below.
> 
> The series also includes updates for Simatic IPC drivers that partially
> tagged by respective maintainers (the main question is if Pavel is okay
> with the last three patches, since I believe Hans is okay with removing
> some code under PDx86). Hence the first 8 patches can be merged right
> away and the rest when Pavel does his review.

Kernel test bot seems found an issue with dependencies, because selection
of P2SB is not enough.

There are two solutions that I can see now:
1) move P2SB out of X86_PLATFORM_DEVICES section (like PMC_ATOM);
2) add 'depends on X86_PLATFORM_DEVICES' to the affected drivers.

I think the first solution cleaner, because it would be strange to have
the dependency on the drivers that quite unlikely be on server platforms
(e.g. EDAC).

In long term perhaps something like drivers/platform/x86/lib which is for
platform libraries or so and independent on X86_PLATFORM_DEVICES?

I will send a fix soon as per 1) above, feel free to comment here or there.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v6 00/12] platform/x86: introduce p2sb_bar() helper

[Henning Schild]

Am Mon, 18 Jul 2022 14:17:42 +0300
schrieb Andy Shevchenko <andriy.shevchenko@linux.intel.com>:

> On Mon, Jun 06, 2022 at 07:41:26PM +0300, Andy Shevchenko wrote:
> > There are a few users that would like to utilize P2SB mechanism of
> > hiding and unhiding a device from the PCI configuration space.
> > 
> > Here is the series to consolidate p2sb handling code for existing
> > users and to provide a generic way for new comer(s).
> > 
> > It also includes a patch to enable GPIO controllers on Apollo Lake
> > when it's used with ABL bootloader w/o ACPI support.
> > 
> > The patch that brings the helper ("platform/x86/intel: Add Primary
> > to Sideband (P2SB) bridge support") has a commit message that sheds
> > a light on what the P2SB is and why this is needed.
> > 
> > I have tested this on Apollo Lake platform (I'm able to see SPI NOR
> > and since we have an ACPI device for GPIO I do not see any attempts
> > to recreate one).
> > 
> > The series is ready to be merged via MFD tree, but see below.
> > 
> > The series also includes updates for Simatic IPC drivers that
> > partially tagged by respective maintainers (the main question is if
> > Pavel is okay with the last three patches, since I believe Hans is
> > okay with removing some code under PDx86). Hence the first 8
> > patches can be merged right away and the rest when Pavel does his
> > review.  
> 
> Kernel test bot seems found an issue with dependencies, because
> selection of P2SB is not enough.
> 
> There are two solutions that I can see now:
> 1) move P2SB out of X86_PLATFORM_DEVICES section (like PMC_ATOM);
> 2) add 'depends on X86_PLATFORM_DEVICES' to the affected drivers.
> 
> I think the first solution cleaner, because it would be strange to
> have the dependency on the drivers that quite unlikely be on server
> platforms (e.g. EDAC).
> 
> In long term perhaps something like drivers/platform/x86/lib which is
> for platform libraries or so and independent on X86_PLATFORM_DEVICES?
> 
> I will send a fix soon as per 1) above, feel free to comment here or
> there.

Hey Andy,

is that one on the way already? I meanwhile also found a possible
configuration issue in my patches you carry on top. And i suggest to
include or squash
[PATCH] leds: simatic-ipc-leds-gpio: make sure we have the GPIO
providing driver

in this series.

I am also working on adding more models which have GPIO based LEDs. All
the patches are based on this series because it is where i currently
introduce GPIO based LEDs for simatic. I would not want to change the
ordering but at the same time i would like to meet 5.20.

regards,
Henning