[PATCH v4 0/8] platform/x86: introduce p2sb_bar() helper

[PATCH v4 0/8] platform/x86: introduce p2sb_bar() helper

[Andy Shevchenko]

There are a few users and at least one more is coming (*) 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 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 bring 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.

The changes made in v2 do not change the main idea and the functionality
in a big scale. What we need is probably one more (RE-)test done by Henning.
I hope to have it merged to v5.18-rc1 that Siemens can develop their changes
based on this series.

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

*) One in this series, and one is a due after merge in the Simatic IPC drivers

The series may be routed either via MFD (and I guess Lee would prefer that)
or via PDx86, whichever seems better for you, folks. As of today patches
are ACKed by the respective maintainers, but I2C one and one of the MFD.

Wolfram, can you ACK the patch against i2c-i801 driver, if you have no
objections?

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

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/mfd/Kconfig                    |   1 +
 drivers/mfd/lpc_ich.c                  | 136 +++++++++--
 drivers/pinctrl/intel/pinctrl-intel.c  |  14 +-
 drivers/platform/x86/intel/Kconfig     |  12 +
 drivers/platform/x86/intel/Makefile    |   1 +
 drivers/platform/x86/intel/p2sb.c      | 305 +++++++++++++++++++++++++
 include/linux/platform_data/x86/p2sb.h |  27 +++
 11 files changed, 500 insertions(+), 99 deletions(-)
 create mode 100644 drivers/platform/x86/intel/p2sb.c
 create mode 100644 include/linux/platform_data/x86/p2sb.h

-- 
2.34.1

[PATCH v4 1/8] platform/x86/intel: Add Primary to Sideband (P2SB) bridge support

[Andy Shevchenko]

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

There are already two and at least one more user 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.

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>
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    |   1 +
 drivers/platform/x86/intel/p2sb.c      | 299 +++++++++++++++++++++++++
 include/linux/platform_data/x86/p2sb.h |  27 +++
 4 files changed, 339 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 8e65086bb6c8..55b499fb8249 100644
--- a/drivers/platform/x86/intel/Kconfig
+++ b/drivers/platform/x86/intel/Kconfig
@@ -68,6 +68,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 35f2066578b2..6ce944285730 100644
--- a/drivers/platform/x86/intel/Makefile
+++ b/drivers/platform/x86/intel/Makefile
@@ -26,6 +26,7 @@ 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
+obj-$(CONFIG_P2SB)			+= p2sb.o
 intel_vsec-y				:= vsec.o
 obj-$(CONFIG_INTEL_VSEC)		+= intel_vsec.o
 
diff --git a/drivers/platform/x86/intel/p2sb.c b/drivers/platform/x86/intel/p2sb.c
new file mode 100644
index 000000000000..8c12882d0bde
--- /dev/null
+++ b/drivers/platform/x86/intel/p2sb.c
@@ -0,0 +1,299 @@
+// 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/bitops.h>
+#include <linux/export.h>
+#include <linux/pci.h>
+#include <linux/platform_data/x86/p2sb.h>
+#include <linux/printk.h>
+
+#include <asm/cpu_device_id.h>
+#include <asm/intel-family.h>
+
+#define p2sb_printk(level, bus, devfn, fmt, arg...) \
+	printk(level "pci %04x:%02x:%02x.%d: " fmt, \
+	       pci_domain_nr(bus), (bus)->number, PCI_SLOT(devfn), PCI_FUNC(devfn), ##arg)
+
+#define p2sb_err(bus, devfn, fmt, arg...)	p2sb_printk(KERN_ERR, (bus), devfn, fmt, ##arg)
+#define p2sb_info(bus, devfn, fmt, arg...)	p2sb_printk(KERN_INFO, (bus), devfn, fmt, ##arg)
+
+#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;
+}
+
+/*
+ * Here is practically a copy'n'paste of __pci_read_base() and Co,
+ * modified to work with PCI bus and devfn instead of PCI device.
+ *
+ * Note, the PCI core doesn't want to have that being refactored
+ * and reused.
+ */
+static u64 pci_size(u64 base, u64 maxbase, u64 mask)
+{
+	u64 size = mask & maxbase;	/* Find the significant bits */
+	if (!size)
+		return 0;
+
+	/*
+	 * Get the lowest of them to find the decode size, and from that
+	 * the extent.
+	 */
+	size = size & ~(size-1);
+
+	/*
+	 * base == maxbase can be valid only if the BAR has already been
+	 * programmed with all 1s.
+	 */
+	if (base == maxbase && ((base | (size - 1)) & mask) != mask)
+		return 0;
+
+	return size;
+}
+
+static inline unsigned long decode_bar(u32 bar)
+{
+	u32 mem_type;
+	unsigned long flags;
+
+	if ((bar & PCI_BASE_ADDRESS_SPACE) == PCI_BASE_ADDRESS_SPACE_IO) {
+		flags = bar & ~PCI_BASE_ADDRESS_IO_MASK;
+		flags |= IORESOURCE_IO;
+		return flags;
+	}
+
+	flags = bar & ~PCI_BASE_ADDRESS_MEM_MASK;
+	flags |= IORESOURCE_MEM;
+	if (flags & PCI_BASE_ADDRESS_MEM_PREFETCH)
+		flags |= IORESOURCE_PREFETCH;
+
+	mem_type = bar & PCI_BASE_ADDRESS_MEM_TYPE_MASK;
+	switch (mem_type) {
+	case PCI_BASE_ADDRESS_MEM_TYPE_32:
+		break;
+	case PCI_BASE_ADDRESS_MEM_TYPE_1M:
+		/* 1M mem BAR treated as 32-bit BAR */
+		break;
+	case PCI_BASE_ADDRESS_MEM_TYPE_64:
+		flags |= IORESOURCE_MEM_64;
+		break;
+	default:
+		/* mem unknown type treated as 32-bit BAR */
+		break;
+	}
+	return flags;
+}
+
+/**
+ * __pci_bus_read_base - Read a PCI BAR
+ * @bus: the PCI bus
+ * @devfn: the PCI device and function
+ * @res: resource buffer to be filled in
+ * @pos: BAR position in the config space
+ *
+ * Returns 1 if the BAR is 64-bit, or 0 if 32-bit.
+ * In case of error resulting @res->flags is 0.
+ */
+static int __pci_bus_read_base(struct pci_bus *bus, unsigned int devfn,
+			       struct resource *res, unsigned int pos)
+{
+	u32 l = 0, sz = 0, mask = ~0;
+	u64 l64, sz64, mask64;
+	struct pci_bus_region region, inverted_region;
+
+	pci_bus_read_config_dword(bus, devfn, pos, &l);
+	pci_bus_write_config_dword(bus, devfn, pos, l | mask);
+	pci_bus_read_config_dword(bus, devfn, pos, &sz);
+	pci_bus_write_config_dword(bus, devfn, pos, l);
+
+	/*
+	 * All bits set in sz means the device isn't working properly.
+	 * If the BAR isn't implemented, all bits must be 0.  If it's a
+	 * memory BAR or a ROM, bit 0 must be clear; if it's an io BAR, bit
+	 * 1 must be clear.
+	 */
+	if (PCI_POSSIBLE_ERROR(sz))
+		sz = 0;
+
+	/*
+	 * I don't know how l can have all bits set.  Copied from old code.
+	 * Maybe it fixes a bug on some ancient platform.
+	 */
+	if (PCI_POSSIBLE_ERROR(l))
+		l = 0;
+
+	res->flags = decode_bar(l);
+	res->flags |= IORESOURCE_SIZEALIGN;
+	if (res->flags & IORESOURCE_IO) {
+		l64 = l & PCI_BASE_ADDRESS_IO_MASK;
+		sz64 = sz & PCI_BASE_ADDRESS_IO_MASK;
+		mask64 = PCI_BASE_ADDRESS_IO_MASK & (u32)IO_SPACE_LIMIT;
+	} else {
+		l64 = l & PCI_BASE_ADDRESS_MEM_MASK;
+		sz64 = sz & PCI_BASE_ADDRESS_MEM_MASK;
+		mask64 = (u32)PCI_BASE_ADDRESS_MEM_MASK;
+	}
+
+	if (res->flags & IORESOURCE_MEM_64) {
+		pci_bus_read_config_dword(bus, devfn, pos + 4, &l);
+		pci_bus_write_config_dword(bus, devfn, pos + 4, ~0);
+		pci_bus_read_config_dword(bus, devfn, pos + 4, &sz);
+		pci_bus_write_config_dword(bus, devfn, pos + 4, l);
+
+		l64 |= ((u64)l << 32);
+		sz64 |= ((u64)sz << 32);
+		mask64 |= ((u64)~0 << 32);
+	}
+
+	if (!sz64)
+		goto fail;
+
+	sz64 = pci_size(l64, sz64, mask64);
+	if (!sz64) {
+		p2sb_info(bus, devfn, FW_BUG "reg 0x%x: invalid BAR (can't size)\n", pos);
+		goto fail;
+	}
+
+	if (res->flags & IORESOURCE_MEM_64) {
+		if ((sizeof(pci_bus_addr_t) < 8 || sizeof(resource_size_t) < 8)
+		    && sz64 > 0x100000000ULL) {
+			res->flags |= IORESOURCE_UNSET | IORESOURCE_DISABLED;
+			res->start = 0;
+			res->end = 0;
+			p2sb_err(bus, devfn,
+				 "reg 0x%x: can't handle BAR larger than 4GB (size %#010llx)\n",
+				 pos, (unsigned long long)sz64);
+			goto out;
+		}
+
+		if ((sizeof(pci_bus_addr_t) < 8) && l) {
+			/* Above 32-bit boundary; try to reallocate */
+			res->flags |= IORESOURCE_UNSET;
+			res->start = 0;
+			res->end = sz64 - 1;
+			p2sb_info(bus, devfn,
+				  "reg 0x%x: can't handle BAR above 4GB (bus address %#010llx)\n",
+				  pos, (unsigned long long)l64);
+			goto out;
+		}
+	}
+
+	region.start = l64;
+	region.end = l64 + sz64 - 1;
+
+	pcibios_bus_to_resource(bus, res, &region);
+	pcibios_resource_to_bus(bus, &inverted_region, res);
+
+	/*
+	 * If "A" is a BAR value (a bus address), "bus_to_resource(A)" is
+	 * the corresponding resource address (the physical address used by
+	 * the CPU.  Converting that resource address back to a bus address
+	 * should yield the original BAR value:
+	 *
+	 *     resource_to_bus(bus_to_resource(A)) == A
+	 *
+	 * If it doesn't, CPU accesses to "bus_to_resource(A)" will not
+	 * be claimed by the device.
+	 */
+	if (inverted_region.start != region.start) {
+		res->flags |= IORESOURCE_UNSET;
+		res->start = 0;
+		res->end = region.end - region.start;
+		p2sb_info(bus, devfn, "reg 0x%x: initial BAR value %#010llx invalid\n",
+			  pos, (unsigned long long)region.start);
+	}
+
+	goto out;
+
+fail:
+	res->flags = 0;
+out:
+	if (res->flags)
+		p2sb_info(bus, devfn, "reg 0x%x: %pR\n", pos, res);
+
+	return (res->flags & IORESOURCE_MEM_64) ? 1 : 0;
+}
+
+/**
+ * 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 in use.
+ * 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)
+{
+	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 @pdev is NULL, use bus 0 in domain 0 */
+	bus = bus ?: pci_find_bus(0, 0);
+
+	/* If @devfn is 0, replace it with devfn of P2SB device itself */
+	devfn = devfn ?: devfn_p2sb;
+
+	pci_lock_rescan_remove();
+
+	/* Unhide the P2SB device, if needed */
+	pci_bus_read_config_dword(bus, devfn_p2sb, P2SBC, &value);
+	if ((value & P2SBC_HIDE) == P2SBC_HIDE)
+		pci_bus_write_config_dword(bus, devfn_p2sb, P2SBC, 0);
+
+	/* Read the first BAR of the device in question */
+	__pci_bus_read_base(bus, devfn, mem, PCI_BASE_ADDRESS_0);
+
+	/* 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 (mem->flags == 0) {
+		p2sb_err(bus, devfn, "Can't read BAR0: %pR\n", mem);
+		return -ENODEV;
+	}
+
+	p2sb_info(bus, devfn, "BAR: %pR\n", mem);
+	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..2f71de65aee4
--- /dev/null
+++ b/include/linux/platform_data/x86/p2sb.h
@@ -0,0 +1,27 @@
+/* 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>
+
+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.34.1

Re: [PATCH v4 1/8] platform/x86/intel: Add Primary to Sideband (P2SB) bridge support

[Hans de Goede]

Hi Andy,

On 1/31/22 16:13, Andy Shevchenko wrote:
> From: Jonathan Yong <jonathan.yong@intel.com>
> 
> There are already two and at least one more user 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.
> 
> 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>
> 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    |   1 +
>  drivers/platform/x86/intel/p2sb.c      | 299 +++++++++++++++++++++++++
>  include/linux/platform_data/x86/p2sb.h |  27 +++
>  4 files changed, 339 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 8e65086bb6c8..55b499fb8249 100644
> --- a/drivers/platform/x86/intel/Kconfig
> +++ b/drivers/platform/x86/intel/Kconfig
> @@ -68,6 +68,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 35f2066578b2..6ce944285730 100644
> --- a/drivers/platform/x86/intel/Makefile
> +++ b/drivers/platform/x86/intel/Makefile
> @@ -26,6 +26,7 @@ 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
> +obj-$(CONFIG_P2SB)			+= p2sb.o
>  intel_vsec-y				:= vsec.o
>  obj-$(CONFIG_INTEL_VSEC)		+= intel_vsec.o
>  
> diff --git a/drivers/platform/x86/intel/p2sb.c b/drivers/platform/x86/intel/p2sb.c
> new file mode 100644
> index 000000000000..8c12882d0bde
> --- /dev/null
> +++ b/drivers/platform/x86/intel/p2sb.c
> @@ -0,0 +1,299 @@
> +// 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/bitops.h>
> +#include <linux/export.h>
> +#include <linux/pci.h>
> +#include <linux/platform_data/x86/p2sb.h>
> +#include <linux/printk.h>
> +
> +#include <asm/cpu_device_id.h>
> +#include <asm/intel-family.h>
> +
> +#define p2sb_printk(level, bus, devfn, fmt, arg...) \
> +	printk(level "pci %04x:%02x:%02x.%d: " fmt, \
> +	       pci_domain_nr(bus), (bus)->number, PCI_SLOT(devfn), PCI_FUNC(devfn), ##arg)
> +
> +#define p2sb_err(bus, devfn, fmt, arg...)	p2sb_printk(KERN_ERR, (bus), devfn, fmt, ##arg)
> +#define p2sb_info(bus, devfn, fmt, arg...)	p2sb_printk(KERN_INFO, (bus), devfn, fmt, ##arg)
> +
> +#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;
> +}
> +
> +/*
> + * Here is practically a copy'n'paste of __pci_read_base() and Co,
> + * modified to work with PCI bus and devfn instead of PCI device.
> + *
> + * Note, the PCI core doesn't want to have that being refactored
> + * and reused.
> + */
> +static u64 pci_size(u64 base, u64 maxbase, u64 mask)
> +{
> +	u64 size = mask & maxbase;	/* Find the significant bits */
> +	if (!size)
> +		return 0;
> +
> +	/*
> +	 * Get the lowest of them to find the decode size, and from that
> +	 * the extent.
> +	 */
> +	size = size & ~(size-1);
> +
> +	/*
> +	 * base == maxbase can be valid only if the BAR has already been
> +	 * programmed with all 1s.
> +	 */
> +	if (base == maxbase && ((base | (size - 1)) & mask) != mask)
> +		return 0;
> +
> +	return size;
> +}
> +
> +static inline unsigned long decode_bar(u32 bar)
> +{
> +	u32 mem_type;
> +	unsigned long flags;
> +
> +	if ((bar & PCI_BASE_ADDRESS_SPACE) == PCI_BASE_ADDRESS_SPACE_IO) {
> +		flags = bar & ~PCI_BASE_ADDRESS_IO_MASK;
> +		flags |= IORESOURCE_IO;
> +		return flags;
> +	}
> +
> +	flags = bar & ~PCI_BASE_ADDRESS_MEM_MASK;
> +	flags |= IORESOURCE_MEM;
> +	if (flags & PCI_BASE_ADDRESS_MEM_PREFETCH)
> +		flags |= IORESOURCE_PREFETCH;
> +
> +	mem_type = bar & PCI_BASE_ADDRESS_MEM_TYPE_MASK;
> +	switch (mem_type) {
> +	case PCI_BASE_ADDRESS_MEM_TYPE_32:
> +		break;
> +	case PCI_BASE_ADDRESS_MEM_TYPE_1M:
> +		/* 1M mem BAR treated as 32-bit BAR */
> +		break;
> +	case PCI_BASE_ADDRESS_MEM_TYPE_64:
> +		flags |= IORESOURCE_MEM_64;
> +		break;
> +	default:
> +		/* mem unknown type treated as 32-bit BAR */
> +		break;
> +	}
> +	return flags;
> +}
> +
> +/**
> + * __pci_bus_read_base - Read a PCI BAR
> + * @bus: the PCI bus
> + * @devfn: the PCI device and function
> + * @res: resource buffer to be filled in
> + * @pos: BAR position in the config space
> + *
> + * Returns 1 if the BAR is 64-bit, or 0 if 32-bit.
> + * In case of error resulting @res->flags is 0.
> + */
> +static int __pci_bus_read_base(struct pci_bus *bus, unsigned int devfn,
> +			       struct resource *res, unsigned int pos)
> +{

Andy, as discussed already, this is quite a big function to mostly duplicate
from the PCI-core. AFAIK the last status of the discussion surrounding this
was trying to have the P2SB code here create a fake pci_dev struct and use
that with the existing function instead.

Have you given this approach a try ?

Note if that does not work out I'm fine with taking this patch as is.

Regards,

Hans


> +	u32 l = 0, sz = 0, mask = ~0;
> +	u64 l64, sz64, mask64;
> +	struct pci_bus_region region, inverted_region;
> +
> +	pci_bus_read_config_dword(bus, devfn, pos, &l);
> +	pci_bus_write_config_dword(bus, devfn, pos, l | mask);
> +	pci_bus_read_config_dword(bus, devfn, pos, &sz);
> +	pci_bus_write_config_dword(bus, devfn, pos, l);
> +
> +	/*
> +	 * All bits set in sz means the device isn't working properly.
> +	 * If the BAR isn't implemented, all bits must be 0.  If it's a
> +	 * memory BAR or a ROM, bit 0 must be clear; if it's an io BAR, bit
> +	 * 1 must be clear.
> +	 */
> +	if (PCI_POSSIBLE_ERROR(sz))
> +		sz = 0;
> +
> +	/*
> +	 * I don't know how l can have all bits set.  Copied from old code.
> +	 * Maybe it fixes a bug on some ancient platform.
> +	 */
> +	if (PCI_POSSIBLE_ERROR(l))
> +		l = 0;
> +
> +	res->flags = decode_bar(l);
> +	res->flags |= IORESOURCE_SIZEALIGN;
> +	if (res->flags & IORESOURCE_IO) {
> +		l64 = l & PCI_BASE_ADDRESS_IO_MASK;
> +		sz64 = sz & PCI_BASE_ADDRESS_IO_MASK;
> +		mask64 = PCI_BASE_ADDRESS_IO_MASK & (u32)IO_SPACE_LIMIT;
> +	} else {
> +		l64 = l & PCI_BASE_ADDRESS_MEM_MASK;
> +		sz64 = sz & PCI_BASE_ADDRESS_MEM_MASK;
> +		mask64 = (u32)PCI_BASE_ADDRESS_MEM_MASK;
> +	}
> +
> +	if (res->flags & IORESOURCE_MEM_64) {
> +		pci_bus_read_config_dword(bus, devfn, pos + 4, &l);
> +		pci_bus_write_config_dword(bus, devfn, pos + 4, ~0);
> +		pci_bus_read_config_dword(bus, devfn, pos + 4, &sz);
> +		pci_bus_write_config_dword(bus, devfn, pos + 4, l);
> +
> +		l64 |= ((u64)l << 32);
> +		sz64 |= ((u64)sz << 32);
> +		mask64 |= ((u64)~0 << 32);
> +	}
> +
> +	if (!sz64)
> +		goto fail;
> +
> +	sz64 = pci_size(l64, sz64, mask64);
> +	if (!sz64) {
> +		p2sb_info(bus, devfn, FW_BUG "reg 0x%x: invalid BAR (can't size)\n", pos);
> +		goto fail;
> +	}
> +
> +	if (res->flags & IORESOURCE_MEM_64) {
> +		if ((sizeof(pci_bus_addr_t) < 8 || sizeof(resource_size_t) < 8)
> +		    && sz64 > 0x100000000ULL) {
> +			res->flags |= IORESOURCE_UNSET | IORESOURCE_DISABLED;
> +			res->start = 0;
> +			res->end = 0;
> +			p2sb_err(bus, devfn,
> +				 "reg 0x%x: can't handle BAR larger than 4GB (size %#010llx)\n",
> +				 pos, (unsigned long long)sz64);
> +			goto out;
> +		}
> +
> +		if ((sizeof(pci_bus_addr_t) < 8) && l) {
> +			/* Above 32-bit boundary; try to reallocate */
> +			res->flags |= IORESOURCE_UNSET;
> +			res->start = 0;
> +			res->end = sz64 - 1;
> +			p2sb_info(bus, devfn,
> +				  "reg 0x%x: can't handle BAR above 4GB (bus address %#010llx)\n",
> +				  pos, (unsigned long long)l64);
> +			goto out;
> +		}
> +	}
> +
> +	region.start = l64;
> +	region.end = l64 + sz64 - 1;
> +
> +	pcibios_bus_to_resource(bus, res, &region);
> +	pcibios_resource_to_bus(bus, &inverted_region, res);
> +
> +	/*
> +	 * If "A" is a BAR value (a bus address), "bus_to_resource(A)" is
> +	 * the corresponding resource address (the physical address used by
> +	 * the CPU.  Converting that resource address back to a bus address
> +	 * should yield the original BAR value:
> +	 *
> +	 *     resource_to_bus(bus_to_resource(A)) == A
> +	 *
> +	 * If it doesn't, CPU accesses to "bus_to_resource(A)" will not
> +	 * be claimed by the device.
> +	 */
> +	if (inverted_region.start != region.start) {
> +		res->flags |= IORESOURCE_UNSET;
> +		res->start = 0;
> +		res->end = region.end - region.start;
> +		p2sb_info(bus, devfn, "reg 0x%x: initial BAR value %#010llx invalid\n",
> +			  pos, (unsigned long long)region.start);
> +	}
> +
> +	goto out;
> +
> +fail:
> +	res->flags = 0;
> +out:
> +	if (res->flags)
> +		p2sb_info(bus, devfn, "reg 0x%x: %pR\n", pos, res);
> +
> +	return (res->flags & IORESOURCE_MEM_64) ? 1 : 0;
> +}
> +
> +/**
> + * 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 in use.
> + * 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)
> +{
> +	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 @pdev is NULL, use bus 0 in domain 0 */
> +	bus = bus ?: pci_find_bus(0, 0);
> +
> +	/* If @devfn is 0, replace it with devfn of P2SB device itself */
> +	devfn = devfn ?: devfn_p2sb;
> +
> +	pci_lock_rescan_remove();
> +
> +	/* Unhide the P2SB device, if needed */
> +	pci_bus_read_config_dword(bus, devfn_p2sb, P2SBC, &value);
> +	if ((value & P2SBC_HIDE) == P2SBC_HIDE)
> +		pci_bus_write_config_dword(bus, devfn_p2sb, P2SBC, 0);
> +
> +	/* Read the first BAR of the device in question */
> +	__pci_bus_read_base(bus, devfn, mem, PCI_BASE_ADDRESS_0);
> +
> +	/* 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 (mem->flags == 0) {
> +		p2sb_err(bus, devfn, "Can't read BAR0: %pR\n", mem);
> +		return -ENODEV;
> +	}
> +
> +	p2sb_info(bus, devfn, "BAR: %pR\n", mem);
> +	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..2f71de65aee4
> --- /dev/null
> +++ b/include/linux/platform_data/x86/p2sb.h
> @@ -0,0 +1,27 @@
> +/* 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>
> +
> +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 */

Re: [PATCH v4 1/8] platform/x86/intel: Add Primary to Sideband (P2SB) bridge support

[Lukas Wunner]

On Mon, Jan 31, 2022 at 05:13:39PM +0200, Andy Shevchenko wrote:
> Background information
> ======================

The wealth of information in the commit message obscures what the
actual problem is, which is actually quite simple:  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.


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

I find that paragraph confusing:  Do multi-function P2SB devices exist?
What are the other functions?  Are they visible but merely not enumerated
because function 0 is not visible?


> 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

Honestly I would have taken the step to always expose the device,
identify breakages and then fix those.

We had a similar issue with HD audio controllers on Nvidia GPUs
which were only visible when an HDMI cable was plugged in.
We always expose them since b516ea586d71 and I recall we merely
had a few cases that an audio device was exposed in cases when
the card had no HDMI connectors at all.  So there was a useless
HD audio card visible to the user but no real harm.


> +	pci_lock_rescan_remove();
> +
> +	/* Unhide the P2SB device, if needed */
> +	pci_bus_read_config_dword(bus, devfn_p2sb, P2SBC, &value);
> +	if ((value & P2SBC_HIDE) == P2SBC_HIDE)
> +		pci_bus_write_config_dword(bus, devfn_p2sb, P2SBC, 0);
> +
> +	/* Read the first BAR of the device in question */
> +	__pci_bus_read_base(bus, devfn, mem, PCI_BASE_ADDRESS_0);
> +
> +	/* 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();

Please add a code comment why you're calling pci_lock_rescan_remove(),
such as:

	/*
	 * Prevent concurrent PCI bus scan from seeing the P2SB device
	 * while it is temporarily exposed.
	 */

Otherwise it looks like you're abusing that lock to prevent multiple
simultaneous RMW operations of the P2SBC_HIDE bit.


I think the first if-clause above can be simplified to

	if (value & P2SBC_HIDE)

I don't understand why one of the two if-clauses adds "== P2SBC_HIDE".


Do you really need all the complicated logic in __pci_bus_read_base()?
For comparison, simatic_ipc_get_membase0() in simatic-ipc.c merely does:

	pci_bus_read_config_dword(bus, p2sb, PCI_BASE_ADDRESS_0, &bar0);

If that's sufficient for simatic-ipc.c, why is the more complicated code
necessary in p2sb.c?


I'm wondering, since you're only retrieving the base address (and thus
temporarily expose the P2SB device) when it's actually needed by a driver,
would there be any harm in keeping the P2SB device exposed indefinitely
from the moment a driver first requests the base address?  I.e., unhide it
but don't hide it again.  That would allow you to call pci_scan_slot() and
pci_bus_add_devices(), thus instantiating a proper pci_dev which you can
access without the __pci_bus_read_base() kludge.


> +	/*
> +	 * I don't know how l can have all bits set.  Copied from old code.
> +	 * Maybe it fixes a bug on some ancient platform.
> +	 */
> +	if (PCI_POSSIBLE_ERROR(l))
> +		l = 0;

l can have all bits set if the device was hot-removed.  That can't happen
with a built-in device such as P2SB.

Thanks,

Lukas

Re: [PATCH v4 1/8] platform/x86/intel: Add Primary to Sideband (P2SB) bridge support

[Andy Shevchenko]

On Thu, May 5, 2022 at 4:55 PM Lukas Wunner <lukas@wunner.de> wrote:
> On Mon, Jan 31, 2022 at 05:13:39PM +0200, Andy Shevchenko wrote:

Thanks for your review, Lukas! My answers below.

...

> > Background information
> > ======================
>
> The wealth of information in the commit message obscures what the
> actual problem is, which is actually quite simple:  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.

Right, but this long commit message was a result of the previous
discussions with Bjorn. If we're ever going to handle something like
this in the PCI core, perhaps he won't be happy if I remove it. Maybe
we can simply state what you wrote as a problem statement and move
this chapter at the end?

> > 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.
>
> I find that paragraph confusing:  Do multi-function P2SB devices exist?
> What are the other functions?  Are they visible but merely not enumerated
> because function 0 is not visible?

The case I see is when we want to read the BAR from another slot of a
PCI device, 0 function of which is P2SB. Since P2SB is hidden, the
other device is hidden as well. Any idea how to reformulate this? And
yes, we have this in the existing SoCs.

...

> > 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
>
> Honestly I would have taken the step to always expose the device,
> identify breakages and then fix those.

Taking into account the history of the different issues on Intel Atom
SoCs I would like to be on the safer side, that's why we don't want to
expose it and keep the status quo for now. IIRC Hans and Mika are on
the same page with me on this.

> We had a similar issue with HD audio controllers on Nvidia GPUs
> which were only visible when an HDMI cable was plugged in.
> We always expose them since b516ea586d71 and I recall we merely
> had a few cases that an audio device was exposed in cases when
> the card had no HDMI connectors at all.  So there was a useless
> HD audio card visible to the user but no real harm.

...

> > +     pci_unlock_rescan_remove();
>
> Please add a code comment why you're calling pci_lock_rescan_remove(),

Sure!

> such as:
>
>         /*
>          * Prevent concurrent PCI bus scan from seeing the P2SB device
>          * while it is temporarily exposed.
>          */
>
> Otherwise it looks like you're abusing that lock to prevent multiple
> simultaneous RMW operations of the P2SBC_HIDE bit.
>
>
> I think the first if-clause above can be simplified to
>
>         if (value & P2SBC_HIDE)
>
> I don't understand why one of the two if-clauses adds "== P2SBC_HIDE".

Agree.

...

> Do you really need all the complicated logic in __pci_bus_read_base()?
> For comparison, simatic_ipc_get_membase0() in simatic-ipc.c merely does:
>
>         pci_bus_read_config_dword(bus, p2sb, PCI_BASE_ADDRESS_0, &bar0);
>
> If that's sufficient for simatic-ipc.c, why is the more complicated code
> necessary in p2sb.c?

Since it's a PCI device I want to follow what PCI core does with it.
As I explained somewhere that the current code (actually it's a
simplified version of what is done in PCI core) follows what spec
requires. I would like to be in alignment with the spec, while it
still may work with less code. Besides that, it's theoretically
possible that the base address may be 64-bit in new SoCs, I won't
rewrite code again just because we abused the spec.

> I'm wondering, since you're only retrieving the base address (and thus
> temporarily expose the P2SB device) when it's actually needed by a driver,
> would there be any harm in keeping the P2SB device exposed indefinitely
> from the moment a driver first requests the base address?  I.e., unhide it
> but don't hide it again.  That would allow you to call pci_scan_slot() and
> pci_bus_add_devices(), thus instantiating a proper pci_dev which you can
> access without the __pci_bus_read_base() kludge.

Same as above about permanent unhide awareness.
Don't forget that on the same SoCs but with different BIOSes the GPIO,
for example, may or may not be exposed via ACPI, which means that we
need to take precautions with the possible conflicts in device
enumeration (we obviously prefer the ACPI enumeration over P2SB). If
P2SB is always exposed it's theoretically possible and maybe even
practically that base address is changed (by unbinding/binding cycle),
while GPIO is enumerated via P2SB and hence we may end up with the
completely wrong addresses in GPIO.

...

> > +     /*
> > +      * I don't know how l can have all bits set.  Copied from old code.
> > +      * Maybe it fixes a bug on some ancient platform.
> > +      */
> > +     if (PCI_POSSIBLE_ERROR(l))
> > +             l = 0;
>
> l can have all bits set if the device was hot-removed.  That can't happen
> with a built-in device such as P2SB.

Can be dropped, indeed. But that chicken bit emulates that :-) Anyway,
we unhide the device before looking into it, so we shouldn't have the
surprise "removals".

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v4 1/8] platform/x86/intel: Add Primary to Sideband (P2SB) bridge support

[Lukas Wunner]

On Thu, May 05, 2022 at 07:54:49PM +0200, Andy Shevchenko wrote:
> On Thu, May 5, 2022 at 4:55 PM Lukas Wunner <lukas@wunner.de> wrote:
> > On Mon, Jan 31, 2022 at 05:13:39PM +0200, Andy Shevchenko wrote:
> > > Background information
> > > ======================
> >
> > The wealth of information in the commit message obscures what the
> > actual problem is, which is actually quite simple:  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.
> 
> Right, but this long commit message was a result of the previous
> discussions with Bjorn. If we're ever going to handle something like
> this in the PCI core, perhaps he won't be happy if I remove it. Maybe
> we can simply state what you wrote as a problem statement and move
> this chapter at the end?

Yes, feel free to copy-paste the synopsis from my e-mail above
and rephrase as you see fit.


> > > 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.
> >
> > I find that paragraph confusing:  Do multi-function P2SB devices exist?
> > What are the other functions?  Are they visible but merely not enumerated
> > because function 0 is not visible?
> 
> The case I see is when we want to read the BAR from another slot of a
> PCI device, 0 function of which is P2SB. Since P2SB is hidden, the
> other device is hidden as well. Any idea how to reformulate this? And
> yes, we have this in the existing SoCs.

The spec you linked to in the commit message (for the 100 series chipset)
says that P2SB is located at Device 31 Function 1.

In those chipsets where P2SB is function 0, what kind of devices are
at functions 1 and higher?


> > Do you really need all the complicated logic in __pci_bus_read_base()?
> > For comparison, simatic_ipc_get_membase0() in simatic-ipc.c merely does:
> >
> >         pci_bus_read_config_dword(bus, p2sb, PCI_BASE_ADDRESS_0, &bar0);
> >
> > If that's sufficient for simatic-ipc.c, why is the more complicated code
> > necessary in p2sb.c?
> 
> Since it's a PCI device I want to follow what PCI core does with it.
> As I explained somewhere that the current code (actually it's a
> simplified version of what is done in PCI core) follows what spec
> requires. I would like to be in alignment with the spec, while it
> still may work with less code. Besides that, it's theoretically
> possible that the base address may be 64-bit in new SoCs, I won't
> rewrite code again just because we abused the spec.

So as an alternative to copy-pasting __pci_bus_read_base(),
you could just call pci_scan_single_device().  This will create
a proper pci_dev that you can work with.  Note that no driver will
be bound to the device because of:

  pci_scan_single_device()
    pci_device_add()
      dev->match_driver = false

After you've read the BAR, get rid of the pci_dev with pci_destroy_dev().


> > > +     /*
> > > +      * I don't know how l can have all bits set.  Copied from old code.
> > > +      * Maybe it fixes a bug on some ancient platform.
> > > +      */
> > > +     if (PCI_POSSIBLE_ERROR(l))
> > > +             l = 0;
> >
> > l can have all bits set if the device was hot-removed.  That can't happen
> > with a built-in device such as P2SB.
> 
> Can be dropped, indeed. But that chicken bit emulates that :-) Anyway,
> we unhide the device before looking into it, so we shouldn't have the
> surprise "removals".

pci_lock_rescan_remove() prevents concurrent unhiding as well as
removal via sysfs.

Thanks,

Lukas

Re: [PATCH v4 1/8] platform/x86/intel: Add Primary to Sideband (P2SB) bridge support

[Andy Shevchenko]

On Sun, May 8, 2022 at 9:13 AM Lukas Wunner <lukas@wunner.de> wrote:
> On Thu, May 05, 2022 at 07:54:49PM +0200, Andy Shevchenko wrote:
> > On Thu, May 5, 2022 at 4:55 PM Lukas Wunner <lukas@wunner.de> wrote:
> > > On Mon, Jan 31, 2022 at 05:13:39PM +0200, Andy Shevchenko wrote:

...

> > > > Background information
> > > > ======================
> > >
> > > The wealth of information in the commit message obscures what the
> > > actual problem is, which is actually quite simple:  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.
> >
> > Right, but this long commit message was a result of the previous
> > discussions with Bjorn. If we're ever going to handle something like
> > this in the PCI core, perhaps he won't be happy if I remove it. Maybe
> > we can simply state what you wrote as a problem statement and move
> > this chapter at the end?
>
> Yes, feel free to copy-paste the synopsis from my e-mail above
> and rephrase as you see fit.

Will do.

...

> > > > 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.
> > >
> > > I find that paragraph confusing:  Do multi-function P2SB devices exist?
> > > What are the other functions?  Are they visible but merely not enumerated
> > > because function 0 is not visible?
> >
> > The case I see is when we want to read the BAR from another slot of a
> > PCI device, 0 function of which is P2SB. Since P2SB is hidden, the
> > other device is hidden as well. Any idea how to reformulate this? And
> > yes, we have this in the existing SoCs.
>
> The spec you linked to in the commit message (for the 100 series chipset)
> says that P2SB is located at Device 31 Function 1.
>
> In those chipsets where P2SB is function 0, what kind of devices are
> at functions 1 and higher?

In the Intel Broxton and Apollo Lake cases the P2SB is the function 0
and we want to have a BAR of SPI NOR, which is function 2.

...

> > > Do you really need all the complicated logic in __pci_bus_read_base()?
> > > For comparison, simatic_ipc_get_membase0() in simatic-ipc.c merely does:
> > >
> > >         pci_bus_read_config_dword(bus, p2sb, PCI_BASE_ADDRESS_0, &bar0);
> > >
> > > If that's sufficient for simatic-ipc.c, why is the more complicated code
> > > necessary in p2sb.c?
> >
> > Since it's a PCI device I want to follow what PCI core does with it.
> > As I explained somewhere that the current code (actually it's a
> > simplified version of what is done in PCI core) follows what spec
> > requires. I would like to be in alignment with the spec, while it
> > still may work with less code. Besides that, it's theoretically
> > possible that the base address may be 64-bit in new SoCs, I won't
> > rewrite code again just because we abused the spec.
>
> So as an alternative to copy-pasting __pci_bus_read_base(),
> you could just call pci_scan_single_device().  This will create
> a proper pci_dev that you can work with.  Note that no driver will
> be bound to the device because of:
>
>   pci_scan_single_device()
>     pci_device_add()
>       dev->match_driver = false
>
> After you've read the BAR, get rid of the pci_dev with pci_destroy_dev().

This is pretty nice, if it flies! I definitely try this ASAP (during
working hours).
Thanks for the hint.

...

> > > > +     /*
> > > > +      * I don't know how l can have all bits set.  Copied from old code.
> > > > +      * Maybe it fixes a bug on some ancient platform.
> > > > +      */
> > > > +     if (PCI_POSSIBLE_ERROR(l))
> > > > +             l = 0;
> > >
> > > l can have all bits set if the device was hot-removed.  That can't happen
> > > with a built-in device such as P2SB.
> >
> > Can be dropped, indeed. But that chicken bit emulates that :-) Anyway,
> > we unhide the device before looking into it, so we shouldn't have the
> > surprise "removals".
>
> pci_lock_rescan_remove() prevents concurrent unhiding as well as
> removal via sysfs.

Yep, that's good. In any case this piece of code will be gone if your
above suggestion works, have I got it right?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v4 1/8] platform/x86/intel: Add Primary to Sideband (P2SB) bridge support

[Lukas Wunner]

On Sun, May 08, 2022 at 12:05:53PM +0200, Andy Shevchenko wrote:
> On Sun, May 8, 2022 at 9:13 AM Lukas Wunner <lukas@wunner.de> wrote:
> > pci_lock_rescan_remove() prevents concurrent unhiding as well as
> > removal via sysfs.
> 
> Yep, that's good. In any case this piece of code will be gone if your
> above suggestion works, have I got it right?

Yes.  You just need to make sure that you call pci_scan_single_device()
*after* unhiding the P2SB device so that this check succeeds:

  pci_scan_single_device()
    pci_scan_device()
      pci_bus_read_dev_vendor_id()

Thanks,

Lukas

[PATCH v4 2/8] 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>
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 826d494f3cc6..48f55991ae8c 100644
--- a/drivers/pinctrl/intel/pinctrl-intel.c
+++ b/drivers/pinctrl/intel/pinctrl-intel.c
@@ -1626,16 +1626,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];
@@ -1649,7 +1647,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.34.1

[PATCH v4 3/8] 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>
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 | 17 ++++++++++++-----
 1 file changed, 12 insertions(+), 5 deletions(-)

diff --git a/drivers/mfd/lpc_ich.c b/drivers/mfd/lpc_ich.c
index f10e53187f67..13d8c64318e6 100644
--- a/drivers/mfd/lpc_ich.c
+++ b/drivers/mfd/lpc_ich.c
@@ -1084,12 +1084,21 @@ static int lpc_ich_init_wdt(struct pci_dev *dev)
 	return ret;
 }
 
+static void lpc_ich_test_spi_write(struct pci_dev *dev, unsigned int devfn,
+				   struct intel_spi_boardinfo *info)
+{
+	u32 bcr;
+
+	pci_bus_read_config_dword(dev->bus, devfn, BCR, &bcr);
+	info->writeable = !!(bcr & BCR_WPD);
+}
+
 static int lpc_ich_init_spi(struct pci_dev *dev)
 {
 	struct lpc_ich_priv *priv = pci_get_drvdata(dev);
 	struct resource *res = &intel_spi_res[0];
 	struct intel_spi_boardinfo *info;
-	u32 spi_base, rcba, bcr;
+	u32 spi_base, rcba;
 
 	info = devm_kzalloc(&dev->dev, sizeof(*info), GFP_KERNEL);
 	if (!info)
@@ -1113,8 +1122,7 @@ static int lpc_ich_init_spi(struct pci_dev *dev)
 			res->start = spi_base + SPIBASE_LPT;
 			res->end = res->start + SPIBASE_LPT_SZ - 1;
 
-			pci_read_config_dword(dev, BCR, &bcr);
-			info->writeable = !!(bcr & BCR_WPD);
+			lpc_ich_test_spi_write(dev, dev->devfn, info);
 		}
 		break;
 
@@ -1135,8 +1143,7 @@ static int lpc_ich_init_spi(struct pci_dev *dev)
 			res->start = spi_base & 0xfffffff0;
 			res->end = res->start + SPIBASE_APL_SZ - 1;
 
-			pci_bus_read_config_dword(bus, spi, BCR, &bcr);
-			info->writeable = !!(bcr & BCR_WPD);
+			lpc_ich_test_spi_write(dev, spi, info);
 		}
 
 		pci_bus_write_config_byte(bus, p2sb, 0xe1, 0x1);
-- 
2.34.1

[PATCH v4 4/8] 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>
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 | 20 ++++++--------------
 2 files changed, 7 insertions(+), 14 deletions(-)

diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
index ba0b3eb131f1..544a3425c054 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 13d8c64318e6..95dca5434917 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
@@ -69,8 +70,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
@@ -1127,26 +1126,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;
+		int ret;
 
 		/*
 		 * 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;
-
-			lpc_ich_test_spi_write(dev, spi, info);
-		}
+		ret = p2sb_bar(dev->bus, spi, res);
+		if (ret)
+			return ret;
 
-		pci_bus_write_config_byte(bus, p2sb, 0xe1, 0x1);
+		lpc_ich_test_spi_write(dev, spi, info);
 		break;
 	}
 
-- 
2.34.1

[PATCH v4 5/8] 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>
Acked-by: Hans de Goede <hdegoede@redhat.com>
Acked-by: Linus Walleij <linus.walleij@linaro.org>
---
 drivers/mfd/lpc_ich.c | 101 +++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 100 insertions(+), 1 deletion(-)

diff --git a/drivers/mfd/lpc_ich.c b/drivers/mfd/lpc_ich.c
index 95dca5434917..e1bca5325ce7 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>
@@ -140,6 +142,70 @@ 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 */
+#define APL_GPIO_NORTH_OFFSET		0xc50000
+#define APL_GPIO_NORTHWEST_OFFSET	0xc40000
+#define APL_GPIO_WEST_OFFSET		0xc70000
+#define APL_GPIO_SOUTHWEST_OFFSET	0xc00000
+
+#define APL_GPIO_IRQ			14
+
+static struct resource apl_gpio_resources[APL_GPIO_NR_DEVICES][2] = {
+	[APL_GPIO_NORTH] = {
+		DEFINE_RES_MEM(APL_GPIO_NORTH_OFFSET, 0x1000),
+		DEFINE_RES_IRQ(APL_GPIO_IRQ),
+	},
+	[APL_GPIO_NORTHWEST] = {
+		DEFINE_RES_MEM(APL_GPIO_NORTHWEST_OFFSET, 0x1000),
+		DEFINE_RES_IRQ(APL_GPIO_IRQ),
+	},
+	[APL_GPIO_WEST] = {
+		DEFINE_RES_MEM(APL_GPIO_WEST_OFFSET, 0x1000),
+		DEFINE_RES_IRQ(APL_GPIO_IRQ),
+	},
+	[APL_GPIO_SOUTHWEST] = {
+		DEFINE_RES_MEM(APL_GPIO_SOUTHWEST_OFFSET, 0x1000),
+		DEFINE_RES_IRQ(APL_GPIO_IRQ),
+	},
+};
+
+/* The order must be in sync with apl_pinctrl_soc_data */
+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",
@@ -1083,6 +1149,33 @@ 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];
+
+		/* Fill MEM resource */
+		mem->start += base.start;
+		mem->end += base.start;
+		mem->flags = base.flags;
+	}
+
+	return mfd_add_devices(&dev->dev, 0, apl_gpio_devices,
+			       ARRAY_SIZE(apl_gpio_devices), NULL, 0, NULL);
+}
+
 static void lpc_ich_test_spi_write(struct pci_dev *dev, unsigned int devfn,
 				   struct intel_spi_boardinfo *info)
 {
@@ -1199,6 +1292,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.34.1

Re: [PATCH v4 5/8] mfd: lpc_ich: Add support for pinctrl in non-ACPI system

[Lee Jones]

On Mon, 31 Jan 2022, Andy Shevchenko wrote:

> 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>
> Acked-by: Hans de Goede <hdegoede@redhat.com>
> Acked-by: Linus Walleij <linus.walleij@linaro.org>
> ---
>  drivers/mfd/lpc_ich.c | 101 +++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 100 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/mfd/lpc_ich.c b/drivers/mfd/lpc_ich.c
> index 95dca5434917..e1bca5325ce7 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>
> @@ -140,6 +142,70 @@ 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 */
> +#define APL_GPIO_NORTH_OFFSET		0xc50000
> +#define APL_GPIO_NORTHWEST_OFFSET	0xc40000
> +#define APL_GPIO_WEST_OFFSET		0xc70000
> +#define APL_GPIO_SOUTHWEST_OFFSET	0xc00000
> +
> +#define APL_GPIO_IRQ			14
> +
> +static struct resource apl_gpio_resources[APL_GPIO_NR_DEVICES][2] = {
> +	[APL_GPIO_NORTH] = {
> +		DEFINE_RES_MEM(APL_GPIO_NORTH_OFFSET, 0x1000),

Are these 0x1000's being over-written in lpc_ich_init_pinctrl()?

If so, why pre-initialise?

> +		DEFINE_RES_IRQ(APL_GPIO_IRQ),
> +	},
> +	[APL_GPIO_NORTHWEST] = {
> +		DEFINE_RES_MEM(APL_GPIO_NORTHWEST_OFFSET, 0x1000),
> +		DEFINE_RES_IRQ(APL_GPIO_IRQ),
> +	},
> +	[APL_GPIO_WEST] = {
> +		DEFINE_RES_MEM(APL_GPIO_WEST_OFFSET, 0x1000),
> +		DEFINE_RES_IRQ(APL_GPIO_IRQ),
> +	},
> +	[APL_GPIO_SOUTHWEST] = {
> +		DEFINE_RES_MEM(APL_GPIO_SOUTHWEST_OFFSET, 0x1000),
> +		DEFINE_RES_IRQ(APL_GPIO_IRQ),
> +	},
> +};
> +
> +/* The order must be in sync with apl_pinctrl_soc_data */

Why does the order matter if you've pre-enumerated them all?

> +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",
> @@ -1083,6 +1149,33 @@ 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];
> +
> +		/* Fill MEM resource */
> +		mem->start += base.start;
> +		mem->end += base.start;
> +		mem->flags = base.flags;
> +	}
> +
> +	return mfd_add_devices(&dev->dev, 0, apl_gpio_devices,
> +			       ARRAY_SIZE(apl_gpio_devices), NULL, 0, NULL);
> +}
> +
>  static void lpc_ich_test_spi_write(struct pci_dev *dev, unsigned int devfn,
>  				   struct intel_spi_boardinfo *info)
>  {
> @@ -1199,6 +1292,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)

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

Re: [PATCH v4 5/8] mfd: lpc_ich: Add support for pinctrl in non-ACPI system

[Andy Shevchenko]

On Tue, Feb 15, 2022 at 04:54:00PM +0000, Lee Jones wrote:
> On Mon, 31 Jan 2022, Andy Shevchenko wrote:

Thank you for the review, my answers below.

...

> > +static struct resource apl_gpio_resources[APL_GPIO_NR_DEVICES][2] = {
> > +	[APL_GPIO_NORTH] = {
> > +		DEFINE_RES_MEM(APL_GPIO_NORTH_OFFSET, 0x1000),
> 
> Are these 0x1000's being over-written in lpc_ich_init_pinctrl()?
> 
> If so, why pre-initialise?

You mean to pre-initialize the offsets, but leave the length to be added
in the function? It can be done, but it feels inconsistent, since we would
have offsets and lengths in different places for the same thingy. That said,
I prefer current way for the sake of consistency.

> > +		DEFINE_RES_IRQ(APL_GPIO_IRQ),
> > +	},

...

> > +/* The order must be in sync with apl_pinctrl_soc_data */
> 
> Why does the order matter if you've pre-enumerated them all?

Indeed. I will drop the confusing comment in the next version.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v4 5/8] mfd: lpc_ich: Add support for pinctrl in non-ACPI system

[Lee Jones]

On Tue, 15 Feb 2022, Andy Shevchenko wrote:

> On Tue, Feb 15, 2022 at 04:54:00PM +0000, Lee Jones wrote:
> > On Mon, 31 Jan 2022, Andy Shevchenko wrote:
> 
> Thank you for the review, my answers below.
> 
> ...
> 
> > > +static struct resource apl_gpio_resources[APL_GPIO_NR_DEVICES][2] = {
> > > +	[APL_GPIO_NORTH] = {
> > > +		DEFINE_RES_MEM(APL_GPIO_NORTH_OFFSET, 0x1000),
> > 
> > Are these 0x1000's being over-written in lpc_ich_init_pinctrl()?
> > 
> > If so, why pre-initialise?
> 
> You mean to pre-initialize the offsets, but leave the length to be added
> in the function? It can be done, but it feels inconsistent, since we would
> have offsets and lengths in different places for the same thingy. That said,
> I prefer current way for the sake of consistency.

Don't you over-write this entry entirely?

  for (i = 0; i < ARRAY_SIZE(apl_gpio_devices); i++) {
        struct resource *mem = &apl_gpio_resources[i][0];

        /* Fill MEM resource */
        mem->start += base.start;
        mem->end += base.start;
        mem->flags = base.flags;
  }

Oh wait, you're just adding the base value to the offsets.

In which case that comment is also confusing!

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

Re: [PATCH v4 5/8] mfd: lpc_ich: Add support for pinctrl in non-ACPI system

[Andy Shevchenko]

On Tue, Feb 15, 2022 at 05:29:51PM +0000, Lee Jones wrote:
> On Tue, 15 Feb 2022, Andy Shevchenko wrote:
> > On Tue, Feb 15, 2022 at 04:54:00PM +0000, Lee Jones wrote:
> > > On Mon, 31 Jan 2022, Andy Shevchenko wrote:

...

> > > > +static struct resource apl_gpio_resources[APL_GPIO_NR_DEVICES][2] = {
> > > > +	[APL_GPIO_NORTH] = {
> > > > +		DEFINE_RES_MEM(APL_GPIO_NORTH_OFFSET, 0x1000),
> > > 
> > > Are these 0x1000's being over-written in lpc_ich_init_pinctrl()?
> > > 
> > > If so, why pre-initialise?
> > 
> > You mean to pre-initialize the offsets, but leave the length to be added
> > in the function? It can be done, but it feels inconsistent, since we would
> > have offsets and lengths in different places for the same thingy. That said,
> > I prefer current way for the sake of consistency.
> 
> Don't you over-write this entry entirely?
> 
>   for (i = 0; i < ARRAY_SIZE(apl_gpio_devices); i++) {
>         struct resource *mem = &apl_gpio_resources[i][0];
> 
>         /* Fill MEM resource */
>         mem->start += base.start;
>         mem->end += base.start;
>         mem->flags = base.flags;
>   }
> 
> Oh wait, you're just adding the base value to the offsets.

Right.

> In which case that comment is also confusing!

I will fix a comment in the next version.


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v4 5/8] mfd: lpc_ich: Add support for pinctrl in non-ACPI system

[Andy Shevchenko]

On Tue, Feb 15, 2022 at 05:29:51PM +0000, Lee Jones wrote:
> On Tue, 15 Feb 2022, Andy Shevchenko wrote:
> > On Tue, Feb 15, 2022 at 04:54:00PM +0000, Lee Jones wrote:
> > > On Mon, 31 Jan 2022, Andy Shevchenko wrote:

> > Thank you for the review, my answers below.

...

> > > > +static struct resource apl_gpio_resources[APL_GPIO_NR_DEVICES][2] = {
> > > > +	[APL_GPIO_NORTH] = {
> > > > +		DEFINE_RES_MEM(APL_GPIO_NORTH_OFFSET, 0x1000),
> > > 
> > > Are these 0x1000's being over-written in lpc_ich_init_pinctrl()?
> > > 
> > > If so, why pre-initialise?
> > 
> > You mean to pre-initialize the offsets, but leave the length to be added
> > in the function? It can be done, but it feels inconsistent, since we would
> > have offsets and lengths in different places for the same thingy. That said,
> > I prefer current way for the sake of consistency.
> 
> Don't you over-write this entry entirely?
> 
>   for (i = 0; i < ARRAY_SIZE(apl_gpio_devices); i++) {
>         struct resource *mem = &apl_gpio_resources[i][0];
> 
>         /* Fill MEM resource */
>         mem->start += base.start;
>         mem->end += base.start;
>         mem->flags = base.flags;
>   }
> 
> Oh wait, you're just adding the base value to the offsets.
> 
> In which case that comment is also confusing!

I have realised that in current form it has a bug (*), so I re-do a bit the way
that comment stays and the actual actions will be to *fill* the resource.

*) unbinding and binding back will bring us to the completely wrong resources.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v4 5/8] mfd: lpc_ich: Add support for pinctrl in non-ACPI system

[Henning Schild]

Can this patch not be proposed separately? Maybe i am wrong but it
seems unrelated to the p2sb story.

The whole p2sb base and size discovery is easy and switching the
simatic drivers is also. It is an interface change, where the old open
coding remains working.

But having to switch to GPIO in the same series is kind of weird. That
is a functional change which even might deserve its own cover letter. I
bet there are tons of out-of-tree modules which will stop working on
apl after that gets merged.

I still did not understand why apl is special and other boards do not
get their pinctrl brought up without ACPI/p2sb-visible.

I have patches floating around, but still would be happy if we could do
one thing at a time.

Or maybe it is strongly coupled and i do not understand why.

regards,
Henning

Am Mon, 31 Jan 2022 17:13:43 +0200
schrieb Andy Shevchenko <andriy.shevchenko@linux.intel.com>:

> 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>
> Acked-by: Hans de Goede <hdegoede@redhat.com>
> Acked-by: Linus Walleij <linus.walleij@linaro.org>
> ---
>  drivers/mfd/lpc_ich.c | 101
> +++++++++++++++++++++++++++++++++++++++++- 1 file changed, 100
> insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/mfd/lpc_ich.c b/drivers/mfd/lpc_ich.c
> index 95dca5434917..e1bca5325ce7 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>
> @@ -140,6 +142,70 @@ 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 */
> +#define APL_GPIO_NORTH_OFFSET		0xc50000
> +#define APL_GPIO_NORTHWEST_OFFSET	0xc40000
> +#define APL_GPIO_WEST_OFFSET		0xc70000
> +#define APL_GPIO_SOUTHWEST_OFFSET	0xc00000
> +
> +#define APL_GPIO_IRQ			14
> +
> +static struct resource apl_gpio_resources[APL_GPIO_NR_DEVICES][2] = {
> +	[APL_GPIO_NORTH] = {
> +		DEFINE_RES_MEM(APL_GPIO_NORTH_OFFSET, 0x1000),
> +		DEFINE_RES_IRQ(APL_GPIO_IRQ),
> +	},
> +	[APL_GPIO_NORTHWEST] = {
> +		DEFINE_RES_MEM(APL_GPIO_NORTHWEST_OFFSET, 0x1000),
> +		DEFINE_RES_IRQ(APL_GPIO_IRQ),
> +	},
> +	[APL_GPIO_WEST] = {
> +		DEFINE_RES_MEM(APL_GPIO_WEST_OFFSET, 0x1000),
> +		DEFINE_RES_IRQ(APL_GPIO_IRQ),
> +	},
> +	[APL_GPIO_SOUTHWEST] = {
> +		DEFINE_RES_MEM(APL_GPIO_SOUTHWEST_OFFSET, 0x1000),
> +		DEFINE_RES_IRQ(APL_GPIO_IRQ),
> +	},
> +};
> +
> +/* The order must be in sync with apl_pinctrl_soc_data */
> +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",
> @@ -1083,6 +1149,33 @@ 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];
> +
> +		/* Fill MEM resource */
> +		mem->start += base.start;
> +		mem->end += base.start;
> +		mem->flags = base.flags;
> +	}
> +
> +	return mfd_add_devices(&dev->dev, 0, apl_gpio_devices,
> +			       ARRAY_SIZE(apl_gpio_devices), NULL,
> 0, NULL); +}
> +
>  static void lpc_ich_test_spi_write(struct pci_dev *dev, unsigned int
> devfn, struct intel_spi_boardinfo *info)
>  {
> @@ -1199,6 +1292,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)

Re: [PATCH v4 5/8] mfd: lpc_ich: Add support for pinctrl in non-ACPI system

[Andy Shevchenko]

On Mon, Mar 7, 2022 at 8:21 PM Henning Schild
<henning.schild@siemens.com> wrote:

Please, do not top-post.

> Can this patch not be proposed separately? Maybe i am wrong but it
> seems unrelated to the p2sb story.

The entire story happens to begin from this very change. The author
(you may see that's not me) proposed the change a long time ago and
AFAIU this is the requirement to have it upstreamed.

> The whole p2sb base and size discovery is easy and switching the
> simatic drivers is also. It is an interface change, where the old open
> coding remains working.
>
> But having to switch to GPIO in the same series is kind of weird. That
> is a functional change which even might deserve its own cover letter. I
> bet there are tons of out-of-tree modules which will stop working on
> apl after that gets merged.

Upstream rarely, if at all, cares about 3rd party modules. From the
upstream point of view the thing (whatever the 3rd party module
supports) wasn't working ("no driver" in upstream) and won't work
(still "no driver" in upstream) after the change, so there may not be
any regression.

> I still did not understand why apl is special and other boards do not
> get their pinctrl brought up without ACPI/p2sb-visible.

The platform is being heavily used by one of our departments in such
configuration with firmwares that may not be fully compatible with
UEFI.They want to support that along with the case when BIOS has no
GPIO device being provided.

> I have patches floating around, but still would be happy if we could do
> one thing at a time.

Either way any new changes must use a pin control driver and the
previous work was accepted only on this basis.

> Or maybe it is strongly coupled and I do not understand why.

That's the initial requirement by our peer departament.

-- 
With Best Regards,
Andy Shevchenko

[PATCH v4 6/8] 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>
Acked-by: Hans de Goede <hdegoede@redhat.com>
Acked-by: Linus Walleij <linus.walleij@linaro.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 42da31c1ab70..286f3b14712b 100644
--- a/drivers/i2c/busses/Kconfig
+++ b/drivers/i2c/busses/Kconfig
@@ -101,6 +101,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 7428cc6af5cc..950a9b444adf 100644
--- a/drivers/i2c/busses/i2c-i801.c
+++ b/drivers/i2c/busses/i2c-i801.c
@@ -110,6 +110,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>
 
@@ -139,7 +140,6 @@
 #define TCOBASE		0x050
 #define TCOCTL		0x054
 
-#define SBREG_BAR		0x10
 #define SBREG_SMBCTRL		0xc6000c
 #define SBREG_SMBCTRL_DNV	0xcf000c
 
@@ -1474,45 +1474,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 8c12882d0bde..644cdee4853b 100644
--- a/drivers/platform/x86/intel/p2sb.c
+++ b/drivers/platform/x86/intel/p2sb.c
@@ -29,6 +29,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.34.1

Re: [PATCH v4 6/8] i2c: i801: convert to use common P2SB accessor

[Jean Delvare]

Hi Andy,

On Mon, 31 Jan 2022 17:13:44 +0200, Andy Shevchenko wrote:
> 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>
> Acked-by: Hans de Goede <hdegoede@redhat.com>
> Acked-by: Linus Walleij <linus.walleij@linaro.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(-)
> (...)

Reviewed-by: Jean Delvare <jdelvare@suse.de>

And thank you for taking the time to write this neat P2SB API and to
convert all the code that was doing the same so far.

-- 
Jean Delvare
SUSE L3 Support

Re: [PATCH v4 6/8] i2c: i801: convert to use common P2SB accessor

[Wolfram Sang]

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

On Mon, Jan 31, 2022 at 05:13:44PM +0200, Andy Shevchenko wrote:
> 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>
> Acked-by: Hans de Goede <hdegoede@redhat.com>
> Acked-by: Linus Walleij <linus.walleij@linaro.org>

Acked-by: Wolfram Sang <wsa@kernel.org>


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

[PATCH v4 7/8] 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>
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.34.1

[PATCH v4 8/8] 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>
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 58ab63642e72..e566d66999a9 100644
--- a/drivers/edac/Kconfig
+++ b/drivers/edac/Kconfig
@@ -262,6 +262,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.34.1

Re: [PATCH v4 0/8] platform/x86: introduce p2sb_bar() helper

[Henning Schild]

Am Mon, 31 Jan 2022 17:13:38 +0200
schrieb Andy Shevchenko <andriy.shevchenko@linux.intel.com>:

> There are a few users and at least one more is coming (*) 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 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 bring 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.
> 
> The changes made in v2 do not change the main idea and the
> functionality in a big scale. What we need is probably one more
> (RE-)test done by Henning. 

Just actually read all this the first time, sorry for that! Will take
care of testing on a few of those Simatic IPCs and write patches to
switch to P2SB ... possibly pinctrl where that might pop up without
ACPI.

I guess p5 will make pinctrl show up on what i would call a 127E ...
apollo lake.

> I hope to have it merged to v5.18-rc1 that
> Siemens can develop their changes based on this series.

Will do.

Sorry for the delay after having passed you i now seem to slow you down.

Henning
 
> 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).
> 
> *) One in this series, and one is a due after merge in the Simatic
> IPC drivers
> 
> The series may be routed either via MFD (and I guess Lee would prefer
> that) or via PDx86, whichever seems better for you, folks. As of
> today patches are ACKed by the respective maintainers, but I2C one
> and one of the MFD.
> 
> Wolfram, can you ACK the patch against i2c-i801 driver, if you have no
> objections?
> 
> 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
> 
> 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/mfd/Kconfig                    |   1 +
>  drivers/mfd/lpc_ich.c                  | 136 +++++++++--
>  drivers/pinctrl/intel/pinctrl-intel.c  |  14 +-
>  drivers/platform/x86/intel/Kconfig     |  12 +
>  drivers/platform/x86/intel/Makefile    |   1 +
>  drivers/platform/x86/intel/p2sb.c      | 305
> +++++++++++++++++++++++++ include/linux/platform_data/x86/p2sb.h |
> 27 +++ 11 files changed, 500 insertions(+), 99 deletions(-)
>  create mode 100644 drivers/platform/x86/intel/p2sb.c
>  create mode 100644 include/linux/platform_data/x86/p2sb.h
> 

[PATCH 0/2] simatic-ipc additions to p2sb apl lake gpio

[Henning Schild]

This switches the simatic-ipc modules to using the p2sb interface
introduced by Andy with "platform/x86: introduce p2sb_bar() helper".

It also switches to one apollo lake device to using gpio leds.

I am kind of hoping Andy will take this on top and propose it in his
series.

Henning Schild (2):
  simatic-ipc: convert to use common P2SB accessor
  leds: simatic-ipc-leds-gpio: add GPIO version of Siemens driver

 drivers/leds/simple/Kconfig                   |  11 ++
 drivers/leds/simple/Makefile                  |   3 +-
 drivers/leds/simple/simatic-ipc-leds-gpio.c   | 108 ++++++++++++++++++
 drivers/leds/simple/simatic-ipc-leds.c        |  77 +------------
 drivers/platform/x86/simatic-ipc.c            |  43 +------
 drivers/watchdog/Kconfig                      |   1 +
 drivers/watchdog/simatic-ipc-wdt.c            |  15 +--
 .../platform_data/x86/simatic-ipc-base.h      |   2 -
 8 files changed, 139 insertions(+), 121 deletions(-)
 create mode 100644 drivers/leds/simple/simatic-ipc-leds-gpio.c

-- 
2.34.1

[PATCH 1/2] simatic-ipc: convert to use common P2SB accessor

[Henning Schild]

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>
---
 drivers/leds/simple/Kconfig                   |  1 +
 drivers/leds/simple/simatic-ipc-leds.c        | 14 +++----
 drivers/platform/x86/simatic-ipc.c            | 38 -------------------
 drivers/watchdog/Kconfig                      |  1 +
 drivers/watchdog/simatic-ipc-wdt.c            | 15 ++++----
 .../platform_data/x86/simatic-ipc-base.h      |  2 -
 6 files changed, 17 insertions(+), 54 deletions(-)

diff --git a/drivers/leds/simple/Kconfig b/drivers/leds/simple/Kconfig
index 9f6a68336659..9293e6b36c75 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 if X86
 	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 ff2c96e73241..215ef5b74236 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 */
-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 */
+struct resource simatic_ipc_led_mem_res = DEFINE_RES_MEM_NAMED(0, 0, KBUILD_MODNAME);
 
 static void *simatic_ipc_led_memory;
 
@@ -143,14 +144,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))
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/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
index c8fa79da23b3..ce44a942fc68 100644
--- a/drivers/watchdog/Kconfig
+++ b/drivers/watchdog/Kconfig
@@ -1628,6 +1628,7 @@ config SIEMENS_SIMATIC_IPC_WDT
 	tristate "Siemens Simatic IPC Watchdog"
 	depends on SIEMENS_SIMATIC_IPC
 	select WATCHDOG_CORE
+	select P2SB if X86
 	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))
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.34.1

Re: [PATCH 1/2] simatic-ipc: convert to use common P2SB accessor

[Henning Schild]

Am Tue, 8 Mar 2022 20:35:21 +0100
schrieb 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>
> ---
>  drivers/leds/simple/Kconfig                   |  1 +
>  drivers/leds/simple/simatic-ipc-leds.c        | 14 +++----
>  drivers/platform/x86/simatic-ipc.c            | 38
> ------------------- drivers/watchdog/Kconfig                      |
> 1 + drivers/watchdog/simatic-ipc-wdt.c            | 15 ++++----
>  .../platform_data/x86/simatic-ipc-base.h      |  2 -
>  6 files changed, 17 insertions(+), 54 deletions(-)
> 
> diff --git a/drivers/leds/simple/Kconfig b/drivers/leds/simple/Kconfig
> index 9f6a68336659..9293e6b36c75 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 if X86
>  	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
> ff2c96e73241..215ef5b74236 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
> */ -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 */ +struct resource simatic_ipc_led_mem_res
> = DEFINE_RES_MEM_NAMED(0, 0, KBUILD_MODNAME); 
>  static void *simatic_ipc_led_memory;
>  
> @@ -143,14 +144,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,

After "mfd: lpc_ich: Add support for pinctrl in non-ACPI system" this
will fail because the region will be claimed by pinctrl, did not check
if it would work with !CONFIG_LPC_SCH.

But not a big deal and was expected to happen. I do have a version that
will devm_ioremap (no region) but that seems too hacky to put here.
After all the next patch will remove all that and switch to GPIO.

If we have to propose things in two series i propose to make
CONFIG_LPC_SCH and CONFIG_LEDS_SIEMENS_SIMATIC_IPC conflicting for
intermediate steps.

regards,
Henning

> res); if (IS_ERR(simatic_ipc_led_memory))
> 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/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
> index c8fa79da23b3..ce44a942fc68 100644
> --- a/drivers/watchdog/Kconfig
> +++ b/drivers/watchdog/Kconfig
> @@ -1628,6 +1628,7 @@ config SIEMENS_SIMATIC_IPC_WDT
>  	tristate "Siemens Simatic IPC Watchdog"
>  	depends on SIEMENS_SIMATIC_IPC
>  	select WATCHDOG_CORE
> +	select P2SB if X86
>  	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))
> 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 2/2] leds: simatic-ipc-leds-gpio: add GPIO version of Siemens driver

[Henning Schild]

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>
---
 drivers/leds/simple/Kconfig                 |  12 ++-
 drivers/leds/simple/Makefile                |   3 +-
 drivers/leds/simple/simatic-ipc-leds-gpio.c | 108 ++++++++++++++++++++
 drivers/leds/simple/simatic-ipc-leds.c      |  75 +-------------
 drivers/platform/x86/simatic-ipc.c          |   5 +-
 5 files changed, 129 insertions(+), 74 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 9293e6b36c75..9d2487908743 100644
--- a/drivers/leds/simple/Kconfig
+++ b/drivers/leds/simple/Kconfig
@@ -3,10 +3,20 @@ config LEDS_SIEMENS_SIMATIC_IPC
 	tristate "LED driver for Siemens Simatic IPCs"
 	depends on LEDS_CLASS
 	depends on SIEMENS_SIMATIC_IPC
-	select P2SB if X86
 	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.
+
+config LEDS_SIEMENS_SIMATIC_IPC_GPIO
+	tristate "LED driver for Siemens Simatic IPCs, GPIO based"
+	depends on SIEMENS_SIMATIC_IPC
+	depends on LEDS_GPIO
+	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-gpio.
diff --git a/drivers/leds/simple/Makefile b/drivers/leds/simple/Makefile
index 8481f1e9e360..e1df74fb5915 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.o
+obj-$(CONFIG_LEDS_SIEMENS_SIMATIC_IPC_GPIO)	+= 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..552b65a72e04
--- /dev/null
+++ b/drivers/leds/simple/simatic-ipc-leds-gpio.c
@@ -0,0 +1,108 @@
+// 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, 0);
+	if (IS_ERR(gpiod)) {
+		err = PTR_ERR(gpiod);
+		goto out;
+	}
+	gpiod_set_value(gpiod, 0);
+	gpiod_put(gpiod);
+
+	/* PM_WDT_OUT */
+	gpiod = gpiod_get_index(&simatic_leds_pdev->dev, NULL, 7, 0);
+	if (IS_ERR(gpiod)) {
+		err = PTR_ERR(gpiod);
+		goto out;
+	}
+	gpiod_set_value(gpiod, 0);
+	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 215ef5b74236..0f0dee053aa1 100644
--- a/drivers/leds/simple/simatic-ipc-leds.c
+++ b/drivers/leds/simple/simatic-ipc-leds.c
@@ -24,7 +24,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 +39,6 @@ static struct simatic_ipc_led simatic_ipc_leds_io[] = {
 	{ }
 };
 
-/* the actual start will be discovered with p2sb, 0 is a placeholder */
-struct resource simatic_ipc_led_mem_res = DEFINE_RES_MEM_NAMED(0, 0, KBUILD_MODNAME);
-
-static void *simatic_ipc_led_memory;
-
-static struct simatic_ipc_led simatic_ipc_leds_mem[] = {
-	{0x500 + 0x1A0, "red:" LED_FUNCTION_STATUS "-1"},
-	{0x500 + 0x1A8, "green:" LED_FUNCTION_STATUS "-1"},
-	{0x500 + 0x1C8, "red:" LED_FUNCTION_STATUS "-2"},
-	{0x500 + 0x1D0, "green:" LED_FUNCTION_STATUS "-2"},
-	{0x500 + 0x1E0, "red:" LED_FUNCTION_STATUS "-3"},
-	{0x500 + 0x198, "green:" LED_FUNCTION_STATUS "-3"},
-	{ }
-};
-
 static struct resource simatic_ipc_led_io_res =
 	DEFINE_RES_IO_NAMED(SIMATIC_IPC_LED_PORT_BASE, SZ_2, KBUILD_MODNAME);
 
@@ -89,27 +74,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);
-
-	u32 *p;
-
-	p = simatic_ipc_led_memory + led->value;
-	*p = (*p & ~1) | (brightness == LED_OFF);
-}
-
-static enum led_brightness simatic_ipc_led_get_mem(struct led_classdev *led_cd)
-{
-	struct simatic_ipc_led *led = cdev_to_led(led_cd);
-
-	u32 *p;
-
-	p = simatic_ipc_led_memory + led->value;
-	return (*p & 1) ? LED_OFF : led_cd->max_brightness;
-}
-
 static int simatic_ipc_leds_probe(struct platform_device *pdev)
 {
 	const struct simatic_ipc_platform *plat = pdev->dev.platform_data;
@@ -117,8 +81,7 @@ static int simatic_ipc_leds_probe(struct platform_device *pdev)
 	struct simatic_ipc_led *ipcled;
 	struct led_classdev *cdev;
 	struct resource *res;
-	int err, type;
-	u32 *p;
+	int err;
 
 	switch (plat->devmode) {
 	case SIMATIC_IPC_DEVICE_227D:
@@ -133,35 +96,10 @@ 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 */
-		p = simatic_ipc_led_memory + 0x500 + 0x1D8; /* PM_WDT_OUT */
-		*p = (*p & ~1);
-		p = simatic_ipc_led_memory + 0x500 + 0x1C0; /* PM_BIOS_BOOT_N */
-		*p = (*p | 1);
-
 		break;
 	default:
 		return -ENODEV;
@@ -169,13 +107,8 @@ static int simatic_ipc_leds_probe(struct platform_device *pdev)
 
 	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.34.1

Re: [PATCH 0/2] simatic-ipc additions to p2sb apl lake gpio

[Andy Shevchenko]

On Tue, Mar 08, 2022 at 08:35:20PM +0100, Henning Schild wrote:
> This switches the simatic-ipc modules to using the p2sb interface
> introduced by Andy with "platform/x86: introduce p2sb_bar() helper".
> 
> It also switches to one apollo lake device to using gpio leds.
> 
> I am kind of hoping Andy will take this on top and propose it in his
> series.

First of all, they are not applicable to my current version [1] of the series
(it maybe something changed in the Simatic drivers upstream, because I have got
conflicts there. For the record, I'm using Linux Next as a base.

Second question is could it be possible to split first patch into three, or it
has to be in one?

[1]: https://gitlab.com/andy-shev/next/-/tree/topic/p2sb-next
It would be nice if you can perform another round of testing.

> Henning Schild (2):
>   simatic-ipc: convert to use common P2SB accessor
>   leds: simatic-ipc-leds-gpio: add GPIO version of Siemens driver
> 
>  drivers/leds/simple/Kconfig                   |  11 ++
>  drivers/leds/simple/Makefile                  |   3 +-
>  drivers/leds/simple/simatic-ipc-leds-gpio.c   | 108 ++++++++++++++++++
>  drivers/leds/simple/simatic-ipc-leds.c        |  77 +------------
>  drivers/platform/x86/simatic-ipc.c            |  43 +------
>  drivers/watchdog/Kconfig                      |   1 +
>  drivers/watchdog/simatic-ipc-wdt.c            |  15 +--
>  .../platform_data/x86/simatic-ipc-base.h      |   2 -
>  8 files changed, 139 insertions(+), 121 deletions(-)
>  create mode 100644 drivers/leds/simple/simatic-ipc-leds-gpio.c
> 
> -- 
> 2.34.1
> 

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 0/2] simatic-ipc additions to p2sb apl lake gpio

[Henning Schild]

Am Wed, 4 May 2022 15:51:01 +0300
schrieb Andy Shevchenko <andriy.shevchenko@linux.intel.com>:

> On Tue, Mar 08, 2022 at 08:35:20PM +0100, Henning Schild wrote:
> > This switches the simatic-ipc modules to using the p2sb interface
> > introduced by Andy with "platform/x86: introduce p2sb_bar() helper".
> > 
> > It also switches to one apollo lake device to using gpio leds.
> > 
> > I am kind of hoping Andy will take this on top and propose it in his
> > series.  
> 
> First of all, they are not applicable to my current version [1] of
> the series (it maybe something changed in the Simatic drivers
> upstream, because I have got conflicts there. For the record, I'm
> using Linux Next as a base.

That is possible, some sparse findings have been fixed lately.

> Second question is could it be possible to split first patch into
> three, or it has to be in one?

I assume one for leds one for wdt and finally drop stuff from platform,
and i will go with that assumption for a next round based on your tree
directly.
Can you explain why that will be useful? While it is kind of a
separation of concerns and subsystems ... it also kind of all belongs
together and needs to be merged in a rather strict order.

regards,
Henning

> [1]: https://gitlab.com/andy-shev/next/-/tree/topic/p2sb-next
> It would be nice if you can perform another round of testing.
> 
> > Henning Schild (2):
> >   simatic-ipc: convert to use common P2SB accessor
> >   leds: simatic-ipc-leds-gpio: add GPIO version of Siemens driver
> > 
> >  drivers/leds/simple/Kconfig                   |  11 ++
> >  drivers/leds/simple/Makefile                  |   3 +-
> >  drivers/leds/simple/simatic-ipc-leds-gpio.c   | 108
> > ++++++++++++++++++ drivers/leds/simple/simatic-ipc-leds.c        |
> > 77 +------------ drivers/platform/x86/simatic-ipc.c            |
> > 43 +------ drivers/watchdog/Kconfig                      |   1 +
> >  drivers/watchdog/simatic-ipc-wdt.c            |  15 +--
> >  .../platform_data/x86/simatic-ipc-base.h      |   2 -
> >  8 files changed, 139 insertions(+), 121 deletions(-)
> >  create mode 100644 drivers/leds/simple/simatic-ipc-leds-gpio.c
> > 
> > -- 
> > 2.34.1
> >   
> 

Re: [PATCH 0/2] simatic-ipc additions to p2sb apl lake gpio

[Andy Shevchenko]

On Wed, May 4, 2022 at 6:16 PM Henning Schild
<henning.schild@siemens.com> wrote:
> Am Wed, 4 May 2022 15:51:01 +0300
> schrieb Andy Shevchenko <andriy.shevchenko@linux.intel.com>:
> > On Tue, Mar 08, 2022 at 08:35:20PM +0100, Henning Schild wrote:

...

> > Second question is could it be possible to split first patch into
> > three, or it has to be in one?
>
> I assume one for leds one for wdt and finally drop stuff from platform,

Yes.

> and i will go with that assumption for a next round based on your tree
> directly.

> Can you explain why that will be useful? While it is kind of a
> separation of concerns and subsystems ... it also kind of all belongs
> together and needs to be merged in a rather strict order.

The main case here is that it's easy to review during upstreaming and
in case of somebody looking into the history. It keeps each of the
changes logically isolated. I.o.w. it adds flexibility, for example
changing ordering of the WDT and LED patches in the series in this
case.

I admit that for _this_ series my arguments are not strong, but I'm
speaking out of general approach. The pattern
  1) add new api
  2) switch driver #1 to it
  ...
  2+n) switch driver #n to it
  3+n) drop old API
is how we do in the Linux kernel, even if the changes are coupled
together from a functional / compile perspective.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 0/2] simatic-ipc additions to p2sb apl lake gpio

[Guenter Roeck]

On Wed, May 04, 2022 at 05:19:51PM +0200, Henning Schild wrote:
> Am Wed, 4 May 2022 15:51:01 +0300
> schrieb Andy Shevchenko <andriy.shevchenko@linux.intel.com>:
> 
> > On Tue, Mar 08, 2022 at 08:35:20PM +0100, Henning Schild wrote:
> > > This switches the simatic-ipc modules to using the p2sb interface
> > > introduced by Andy with "platform/x86: introduce p2sb_bar() helper".
> > > 
> > > It also switches to one apollo lake device to using gpio leds.
> > > 
> > > I am kind of hoping Andy will take this on top and propose it in his
> > > series.  
> > 
> > First of all, they are not applicable to my current version [1] of
> > the series (it maybe something changed in the Simatic drivers
> > upstream, because I have got conflicts there. For the record, I'm
> > using Linux Next as a base.
> 
> That is possible, some sparse findings have been fixed lately.
> 
> > Second question is could it be possible to split first patch into
> > three, or it has to be in one?
> 
> I assume one for leds one for wdt and finally drop stuff from platform,
> and i will go with that assumption for a next round based on your tree
> directly.
> Can you explain why that will be useful? While it is kind of a
> separation of concerns and subsystems ... it also kind of all belongs
> together and needs to be merged in a rather strict order.
> 

That is not really correct. It should be possible to split
the patches and only remove simatic_ipc_get_membase0() after the
other patches have been applied.

On a side note, neither subject nor description of patch 1/2
mention that the patch touches both LED and watchdog code, which
is at the very least bad style.

Guenter

> regards,
> Henning
> 
> > [1]: https://gitlab.com/andy-shev/next/-/tree/topic/p2sb-next
> > It would be nice if you can perform another round of testing.
> > 
> > > Henning Schild (2):
> > >   simatic-ipc: convert to use common P2SB accessor
> > >   leds: simatic-ipc-leds-gpio: add GPIO version of Siemens driver
> > > 
> > >  drivers/leds/simple/Kconfig                   |  11 ++
> > >  drivers/leds/simple/Makefile                  |   3 +-
> > >  drivers/leds/simple/simatic-ipc-leds-gpio.c   | 108
> > > ++++++++++++++++++ drivers/leds/simple/simatic-ipc-leds.c        |
> > > 77 +------------ drivers/platform/x86/simatic-ipc.c            |
> > > 43 +------ drivers/watchdog/Kconfig                      |   1 +
> > >  drivers/watchdog/simatic-ipc-wdt.c            |  15 +--
> > >  .../platform_data/x86/simatic-ipc-base.h      |   2 -
> > >  8 files changed, 139 insertions(+), 121 deletions(-)
> > >  create mode 100644 drivers/leds/simple/simatic-ipc-leds-gpio.c
> > > 
> > > -- 
> > > 2.34.1
> > >   
> > 
> 

Re: [PATCH 0/2] simatic-ipc additions to p2sb apl lake gpio

[Henning Schild]

Am Wed, 4 May 2022 17:19:51 +0200
schrieb Henning Schild <henning.schild@siemens.com>:

> Am Wed, 4 May 2022 15:51:01 +0300
> schrieb Andy Shevchenko <andriy.shevchenko@linux.intel.com>:
> 
> > On Tue, Mar 08, 2022 at 08:35:20PM +0100, Henning Schild wrote:  
> > > This switches the simatic-ipc modules to using the p2sb interface
> > > introduced by Andy with "platform/x86: introduce p2sb_bar()
> > > helper".
> > > 
> > > It also switches to one apollo lake device to using gpio leds.
> > > 
> > > I am kind of hoping Andy will take this on top and propose it in
> > > his series.    
> > 
> > First of all, they are not applicable to my current version [1] of
> > the series (it maybe something changed in the Simatic drivers
> > upstream, because I have got conflicts there. For the record, I'm
> > using Linux Next as a base.  
> 
> That is possible, some sparse findings have been fixed lately.
> 
> > Second question is could it be possible to split first patch into
> > three, or it has to be in one?  
> 
> I assume one for leds one for wdt and finally drop stuff from
> platform, and i will go with that assumption for a next round based
> on your tree directly.
> Can you explain why that will be useful? While it is kind of a
> separation of concerns and subsystems ... it also kind of all belongs
> together and needs to be merged in a rather strict order.
> 
> regards,
> Henning
> 
> > [1]: https://gitlab.com/andy-shev/next/-/tree/topic/p2sb-next
> > It would be nice if you can perform another round of testing.

Just got around to testing this with my patches on top. My stuff will
need some more work before i can send again.

Is this a rebasing branch? 
With efc7d77ea372 ("EDAC, pnd2: convert to use common P2SB accessor")
I am seeing problems while booting ... things do work but still error
messages which probably should not be there.

[    2.215715] gpio gpiochip0: (apollolake-pinctrl.0): not an immutable chip, please consider fixing it!
[    2.217506] broxton-pinctrl apollolake-pinctrl.1: Failed to attach ACPI GPIO chip
[    2.217542] gpio gpiochip1: (apollolake-pinctrl.1): not an immutable chip, please consider fixing it!
[    2.217771] i801_smbus 0000:00:1f.1: Failed to enable SMBus PCI device (-22)
[    2.217788] i801_smbus: probe of 0000:00:1f.1 failed with error -22
[    2.221460] broxton-pinctrl apollolake-pinctrl.2: Failed to attach ACPI GPIO chip
[    2.221482] gpio gpiochip2: (apollolake-pinctrl.2): not an immutable chip, please consider fixing it!
[    2.222010] broxton-pinctrl apollolake-pinctrl.3: Failed to attach ACPI GPIO chip
[    2.222023] gpio gpiochip3: (apollolake-pinctrl.3): not an immutable
chip, please consider fixing it!

regards,
Henning


> > > Henning Schild (2):
> > >   simatic-ipc: convert to use common P2SB accessor
> > >   leds: simatic-ipc-leds-gpio: add GPIO version of Siemens driver
> > > 
> > >  drivers/leds/simple/Kconfig                   |  11 ++
> > >  drivers/leds/simple/Makefile                  |   3 +-
> > >  drivers/leds/simple/simatic-ipc-leds-gpio.c   | 108
> > > ++++++++++++++++++ drivers/leds/simple/simatic-ipc-leds.c        |
> > > 77 +------------ drivers/platform/x86/simatic-ipc.c            |
> > > 43 +------ drivers/watchdog/Kconfig                      |   1 +
> > >  drivers/watchdog/simatic-ipc-wdt.c            |  15 +--
> > >  .../platform_data/x86/simatic-ipc-base.h      |   2 -
> > >  8 files changed, 139 insertions(+), 121 deletions(-)
> > >  create mode 100644 drivers/leds/simple/simatic-ipc-leds-gpio.c
> > > 
> > > -- 
> > > 2.34.1
> > >     
> >   
> 

Re: [PATCH 0/2] simatic-ipc additions to p2sb apl lake gpio

[Andy Shevchenko]

On Tue, May 10, 2022 at 05:30:53PM +0200, Henning Schild wrote:
> Am Wed, 4 May 2022 17:19:51 +0200
> schrieb Henning Schild <henning.schild@siemens.com>:
> > Am Wed, 4 May 2022 15:51:01 +0300
> > schrieb Andy Shevchenko <andriy.shevchenko@linux.intel.com>:
> > > On Tue, Mar 08, 2022 at 08:35:20PM +0100, Henning Schild wrote:

...

> > > [1]: https://gitlab.com/andy-shev/next/-/tree/topic/p2sb-next
> > > It would be nice if you can perform another round of testing.
>
> Just got around to testing this with my patches on top. My stuff will
> need some more work before i can send again.
>
> Is this a rebasing branch?
> With efc7d77ea372 ("EDAC, pnd2: convert to use common P2SB accessor")

It's rebased over and over. I just pushed the same version I have sent as v5.

> I am seeing problems while booting ... things do work but still error
> messages which probably should not be there.

It's okay. This is not related to my stuff, it's a new series from Marc which
enables that (harmless) warning.

> [    2.217506] broxton-pinctrl apollolake-pinctrl.1: Failed to attach ACPI GPIO chip
> [    2.217542] gpio gpiochip1: (apollolake-pinctrl.1): not an immutable chip, please consider fixing it!
> [    2.217771] i801_smbus 0000:00:1f.1: Failed to enable SMBus PCI device (-22)
> [    2.217788] i801_smbus: probe of 0000:00:1f.1 failed with error -22
> [    2.221460] broxton-pinctrl apollolake-pinctrl.2: Failed to attach ACPI GPIO chip
> [    2.221482] gpio gpiochip2: (apollolake-pinctrl.2): not an immutable chip, please consider fixing it!
> [    2.222010] broxton-pinctrl apollolake-pinctrl.3: Failed to attach ACPI GPIO chip
> [    2.222023] gpio gpiochip3: (apollolake-pinctrl.3): not an immutable
> chip, please consider fixing it!

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v4 0/8] platform/x86: introduce p2sb_bar() helper

[Henning Schild]

Am Mon, 31 Jan 2022 17:13:38 +0200
schrieb Andy Shevchenko <andriy.shevchenko@linux.intel.com>:

> There are a few users and at least one more is coming (*) 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 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 bring 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.
> 
> The changes made in v2 do not change the main idea and the
> functionality in a big scale. What we need is probably one more
> (RE-)test done by Henning. I hope to have it merged to v5.18-rc1 that
> Siemens can develop their changes based on this series.

I did test this series and it works as expected. Only problem is that
the leds driver will not work together with the pinctrl. Because two
"in tree drivers" will try to reserve the same memory region when both
are enabled. Who wins is a matter of probing order ...

If you can take my changes into your series we will not have a problem.
Otherwise we might need to create sort of a conflict which my series
would revert when switching apl lake to gpio.

I would not know the process, let us see what the reviews bring and how
to continue here.

Thanks so much for taking care, especially the pinctrl coming up
without ACPI really improves the simatic leds on the apl lake.

In fact i will have to double check if i really need the p2sb for the
427E wdt ... but until i have an answer, p2sb works just fine.

regards,
Henning

> 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).
> 
> *) One in this series, and one is a due after merge in the Simatic
> IPC drivers
> 
> The series may be routed either via MFD (and I guess Lee would prefer
> that) or via PDx86, whichever seems better for you, folks. As of
> today patches are ACKed by the respective maintainers, but I2C one
> and one of the MFD.
> 
> Wolfram, can you ACK the patch against i2c-i801 driver, if you have no
> objections?
> 
> 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
> 
> 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/mfd/Kconfig                    |   1 +
>  drivers/mfd/lpc_ich.c                  | 136 +++++++++--
>  drivers/pinctrl/intel/pinctrl-intel.c  |  14 +-
>  drivers/platform/x86/intel/Kconfig     |  12 +
>  drivers/platform/x86/intel/Makefile    |   1 +
>  drivers/platform/x86/intel/p2sb.c      | 305
> +++++++++++++++++++++++++ include/linux/platform_data/x86/p2sb.h |
> 27 +++ 11 files changed, 500 insertions(+), 99 deletions(-)
>  create mode 100644 drivers/platform/x86/intel/p2sb.c
>  create mode 100644 include/linux/platform_data/x86/p2sb.h
> 

Re: [PATCH v4 0/8] platform/x86: introduce p2sb_bar() helper

[Andy Shevchenko]

On Tue, Mar 08, 2022 at 08:50:16PM +0100, Henning Schild wrote:
> Am Mon, 31 Jan 2022 17:13:38 +0200
> schrieb Andy Shevchenko <andriy.shevchenko@linux.intel.com>:
> 
> > There are a few users and at least one more is coming (*) 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 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 bring 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.
> > 
> > The changes made in v2 do not change the main idea and the
> > functionality in a big scale. What we need is probably one more
> > (RE-)test done by Henning. I hope to have it merged to v5.18-rc1 that
> > Siemens can develop their changes based on this series.
> 
> I did test this series and it works as expected. Only problem is that
> the leds driver will not work together with the pinctrl. Because two
> "in tree drivers" will try to reserve the same memory region when both
> are enabled. Who wins is a matter of probing order ...

Can we have your formal Tested-by tag?

> If you can take my changes into your series we will not have a problem.

Yes, that's the plan, but your patches needs a bit of work I believe.

> Otherwise we might need to create sort of a conflict which my series
> would revert when switching apl lake to gpio.
> 
> I would not know the process, let us see what the reviews bring and how
> to continue here.

I'm about to comment on the patches.

> Thanks so much for taking care, especially the pinctrl coming up
> without ACPI really improves the simatic leds on the apl lake.

You are welcome!

> In fact i will have to double check if i really need the p2sb for the
> 427E wdt ... but until i have an answer, p2sb works just fine.

Thanks!

> > 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).
> > 
> > *) One in this series, and one is a due after merge in the Simatic
> > IPC drivers
> > 
> > The series may be routed either via MFD (and I guess Lee would prefer
> > that) or via PDx86, whichever seems better for you, folks. As of
> > today patches are ACKed by the respective maintainers, but I2C one
> > and one of the MFD.
> > 
> > Wolfram, can you ACK the patch against i2c-i801 driver, if you have no
> > objections?
> > 
> > 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
> > 
> > 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/mfd/Kconfig                    |   1 +
> >  drivers/mfd/lpc_ich.c                  | 136 +++++++++--
> >  drivers/pinctrl/intel/pinctrl-intel.c  |  14 +-
> >  drivers/platform/x86/intel/Kconfig     |  12 +
> >  drivers/platform/x86/intel/Makefile    |   1 +
> >  drivers/platform/x86/intel/p2sb.c      | 305
> > +++++++++++++++++++++++++ include/linux/platform_data/x86/p2sb.h |
> > 27 +++ 11 files changed, 500 insertions(+), 99 deletions(-)
> >  create mode 100644 drivers/platform/x86/intel/p2sb.c
> >  create mode 100644 include/linux/platform_data/x86/p2sb.h
> > 
> 

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v4 0/8] platform/x86: introduce p2sb_bar() helper

[Henning Schild]

Am Wed, 4 May 2022 15:42:29 +0300
schrieb Andy Shevchenko <andriy.shevchenko@linux.intel.com>:

> On Tue, Mar 08, 2022 at 08:50:16PM +0100, Henning Schild wrote:
> > Am Mon, 31 Jan 2022 17:13:38 +0200
> > schrieb Andy Shevchenko <andriy.shevchenko@linux.intel.com>:
> >   
> > > There are a few users and at least one more is coming (*) 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 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 bring 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.
> > > 
> > > The changes made in v2 do not change the main idea and the
> > > functionality in a big scale. What we need is probably one more
> > > (RE-)test done by Henning. I hope to have it merged to v5.18-rc1
> > > that Siemens can develop their changes based on this series.  
> > 
> > I did test this series and it works as expected. Only problem is
> > that the leds driver will not work together with the pinctrl.
> > Because two "in tree drivers" will try to reserve the same memory
> > region when both are enabled. Who wins is a matter of probing order
> > ...  
> 
> Can we have your formal Tested-by tag?

Sure. I will just put it here for you to take.

Tested-by: Henning Schild <henning.schild@siemens.com>

Let me know if you need it in another shape or form.

Henning

> 
> > If you can take my changes into your series we will not have a
> > problem.  
> 
> Yes, that's the plan, but your patches needs a bit of work I believe.
> 
> > Otherwise we might need to create sort of a conflict which my series
> > would revert when switching apl lake to gpio.
> > 
> > I would not know the process, let us see what the reviews bring and
> > how to continue here.  
> 
> I'm about to comment on the patches.
> 
> > Thanks so much for taking care, especially the pinctrl coming up
> > without ACPI really improves the simatic leds on the apl lake.  
> 
> You are welcome!
> 
> > In fact i will have to double check if i really need the p2sb for
> > the 427E wdt ... but until i have an answer, p2sb works just fine.  
> 
> Thanks!
> 
> > > 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).
> > > 
> > > *) One in this series, and one is a due after merge in the Simatic
> > > IPC drivers
> > > 
> > > The series may be routed either via MFD (and I guess Lee would
> > > prefer that) or via PDx86, whichever seems better for you, folks.
> > > As of today patches are ACKed by the respective maintainers, but
> > > I2C one and one of the MFD.
> > > 
> > > Wolfram, can you ACK the patch against i2c-i801 driver, if you
> > > have no objections?
> > > 
> > > 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
> > > 
> > > 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/mfd/Kconfig                    |   1 +
> > >  drivers/mfd/lpc_ich.c                  | 136 +++++++++--
> > >  drivers/pinctrl/intel/pinctrl-intel.c  |  14 +-
> > >  drivers/platform/x86/intel/Kconfig     |  12 +
> > >  drivers/platform/x86/intel/Makefile    |   1 +
> > >  drivers/platform/x86/intel/p2sb.c      | 305
> > > +++++++++++++++++++++++++ include/linux/platform_data/x86/p2sb.h |
> > > 27 +++ 11 files changed, 500 insertions(+), 99 deletions(-)
> > >  create mode 100644 drivers/platform/x86/intel/p2sb.c
> > >  create mode 100644 include/linux/platform_data/x86/p2sb.h
> > >   
> >   
> 

Re: [PATCH v4 0/8] platform/x86: introduce p2sb_bar() helper

[Andy Shevchenko]

On Wed, May 4, 2022 at 5:10 PM Henning Schild
<henning.schild@siemens.com> wrote:
> Am Wed, 4 May 2022 15:42:29 +0300
> schrieb Andy Shevchenko <andriy.shevchenko@linux.intel.com>:
> > On Tue, Mar 08, 2022 at 08:50:16PM +0100, Henning Schild wrote:

...

> > Can we have your formal Tested-by tag?
>
> Sure. I will just put it here for you to take.
>
> Tested-by: Henning Schild <henning.schild@siemens.com>

Thank you!

> Let me know if you need it in another shape or form.

Form is okay, just would be nice if you can retest what I have in the
branch as an updated version.

--
With Best Regards,
Andy Shevchenko

Re: [PATCH v4 0/8] platform/x86: introduce p2sb_bar() helper

[Andy Shevchenko]

On Wed, May 04, 2022 at 05:55:26PM +0200, Andy Shevchenko wrote:
> On Wed, May 4, 2022 at 5:10 PM Henning Schild
> <henning.schild@siemens.com> wrote:
> > Am Wed, 4 May 2022 15:42:29 +0300
> > schrieb Andy Shevchenko <andriy.shevchenko@linux.intel.com>:

> ...

> > Let me know if you need it in another shape or form.
> 
> Form is okay, just would be nice if you can retest what I have in the
> branch as an updated version.

Is there any news? I would like to send a new version sooner than later.
We may also apply the patches from this series and when you will be ready
apply yours.

-- 
With Best Regards,
Andy Shevchenko