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

[PATCH v3 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 deduplicate existing users and provide
a generic way for new comers.

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.

Please, comment on the approach and individual patches.

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

(Since it's cross subsystem, the PDx86 seems the main one and
I think it makes sense to route it throught it with immutable
tag or branch provided for the others).

Bjorn, are you okay with this approach and the commit message in the main
patch?

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):
  PCI: Introduce pci_bus_*() printing macros when device is not
    available
  PCI: Convert __pci_read_base() to __pci_bus_read_base()
  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

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/i2c/busses/Kconfig             |   1 +
 drivers/i2c/busses/i2c-i801.c          |  39 ++-----
 drivers/mfd/Kconfig                    |   1 +
 drivers/mfd/lpc_ich.c                  | 136 +++++++++++++++++++++----
 drivers/pci/pci.h                      |  16 ++-
 drivers/pci/probe.c                    |  81 +++++++--------
 drivers/pinctrl/intel/pinctrl-intel.c  |  14 ++-
 drivers/platform/x86/intel/Kconfig     |  12 +++
 drivers/platform/x86/intel/Makefile    |   2 +
 drivers/platform/x86/intel/p2sb.c      |  99 ++++++++++++++++++
 include/linux/pci.h                    |   8 ++
 include/linux/platform_data/x86/p2sb.h |  27 +++++
 12 files changed, 335 insertions(+), 101 deletions(-)
 create mode 100644 drivers/platform/x86/intel/p2sb.c
 create mode 100644 include/linux/platform_data/x86/p2sb.h

-- 
2.34.1

[PATCH v3 1/8] PCI: Introduce pci_bus_*() printing macros when device is not available

[Andy Shevchenko]

In some cases PCI device structure is not available and we want to print
information based on the bus and devfn parameters. For this cases introduce
pci_bus_*() printing macros and replace in existing users.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Reviewed-by: Jean Delvare <jdelvare@suse.de>
---
 drivers/pci/probe.c | 12 +++---------
 include/linux/pci.h |  8 ++++++++
 2 files changed, 11 insertions(+), 9 deletions(-)

diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index 78962652f5bf..82014b248f4d 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -2333,16 +2333,12 @@ static bool pci_bus_wait_crs(struct pci_bus *bus, int devfn, u32 *l,
 	 */
 	while (pci_bus_crs_vendor_id(*l)) {
 		if (delay > timeout) {
-			pr_warn("pci %04x:%02x:%02x.%d: not ready after %dms; giving up\n",
-				pci_domain_nr(bus), bus->number,
-				PCI_SLOT(devfn), PCI_FUNC(devfn), delay - 1);
+			pci_bus_warn(bus, devfn, "not ready after %dms; giving up\n", delay - 1);
 
 			return false;
 		}
 		if (delay >= 1000)
-			pr_info("pci %04x:%02x:%02x.%d: not ready after %dms; waiting\n",
-				pci_domain_nr(bus), bus->number,
-				PCI_SLOT(devfn), PCI_FUNC(devfn), delay - 1);
+			pci_bus_info(bus, devfn, "not ready after %dms; waiting\n", delay - 1);
 
 		msleep(delay);
 		delay *= 2;
@@ -2352,9 +2348,7 @@ static bool pci_bus_wait_crs(struct pci_bus *bus, int devfn, u32 *l,
 	}
 
 	if (delay >= 1000)
-		pr_info("pci %04x:%02x:%02x.%d: ready after %dms\n",
-			pci_domain_nr(bus), bus->number,
-			PCI_SLOT(devfn), PCI_FUNC(devfn), delay - 1);
+		pci_bus_info(bus, devfn, "ready after %dms\n", delay - 1);
 
 	return true;
 }
diff --git a/include/linux/pci.h b/include/linux/pci.h
index d4308f847e58..e3c9edd103df 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -2484,4 +2484,12 @@ void pci_uevent_ers(struct pci_dev *pdev, enum  pci_ers_result err_type);
 	WARN_ONCE(condition, "%s %s: " fmt, \
 		  dev_driver_string(&(pdev)->dev), pci_name(pdev), ##arg)
 
+#define pci_bus_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 pci_bus_err(bus, devfn, fmt, arg...)	pci_bus_printk(KERN_ERR, (bus), devfn, fmt, ##arg)
+#define pci_bus_warn(bus, devfn, fmt, arg...)	pci_bus_printk(KERN_WARNING, (bus), devfn, fmt, ##arg)
+#define pci_bus_info(bus, devfn, fmt, arg...)	pci_bus_printk(KERN_INFO, (bus), devfn, fmt, ##arg)
+
 #endif /* LINUX_PCI_H */
-- 
2.34.1

[PATCH v3 2/8] PCI: Convert __pci_read_base() to __pci_bus_read_base()

[Andy Shevchenko]

Some drivers would like to read PCI BAR of the devices which has been not or
can't be enumerated.

In particular such mechanism is required to read PCI BAR of hidden
devices behind Primary to Sideband (P2SB) bridge.

Refactor __pci_read_base() to provide __pci_bus_read_base() and
represent the former one as static inline helper.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/pci/pci.h   | 16 +++++++++--
 drivers/pci/probe.c | 69 +++++++++++++++++++++++----------------------
 2 files changed, 50 insertions(+), 35 deletions(-)

diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
index 3d60cabde1a1..a03fd2da89b6 100644
--- a/drivers/pci/pci.h
+++ b/drivers/pci/pci.h
@@ -233,8 +233,20 @@ bool pci_bus_generic_read_dev_vendor_id(struct pci_bus *bus, int devfn, u32 *pl,
 int pci_idt_bus_quirk(struct pci_bus *bus, int devfn, u32 *pl, int crs_timeout);
 
 int pci_setup_device(struct pci_dev *dev);
-int __pci_read_base(struct pci_dev *dev, enum pci_bar_type type,
-		    struct resource *res, unsigned int reg);
+
+int __pci_bus_read_base(struct pci_bus *bus, unsigned int devfn,
+			enum pci_bar_type type,
+			struct resource *res, unsigned int reg,
+			bool mmio_always_on);
+static inline int __pci_read_base(struct pci_dev *dev, enum pci_bar_type type,
+				  struct resource *res, unsigned int reg)
+{
+	res->name = pci_name(dev);
+
+	return __pci_bus_read_base(dev->bus, dev->devfn, type, res, reg,
+				   dev->mmio_always_on);
+}
+
 void pci_configure_ari(struct pci_dev *dev);
 void __pci_bus_size_bridges(struct pci_bus *bus,
 			struct list_head *realloc_head);
diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index 82014b248f4d..a5b28073822f 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -130,7 +130,7 @@ static u64 pci_size(u64 base, u64 maxbase, u64 mask)
 	return size;
 }
 
-static inline unsigned long decode_bar(struct pci_dev *dev, u32 bar)
+static inline unsigned long decode_bar(u32 bar)
 {
 	u32 mem_type;
 	unsigned long flags;
@@ -166,16 +166,21 @@ static inline unsigned long decode_bar(struct pci_dev *dev, u32 bar)
 #define PCI_COMMAND_DECODE_ENABLE	(PCI_COMMAND_MEMORY | PCI_COMMAND_IO)
 
 /**
- * __pci_read_base - Read a PCI BAR
- * @dev: the PCI device
+ * __pci_bus_read_base - Read a PCI BAR
+ * @bus: the PCI bus
+ * @devfn: the PCI device and function
  * @type: type of the BAR
  * @res: resource buffer to be filled in
  * @pos: BAR position in the config space
+ * @mmio_always_on: disallow turning off IO/MEM decoding during BAR sizing
  *
  * Returns 1 if the BAR is 64-bit, or 0 if 32-bit.
+ * In case of error resulting @res->flags is 0.
  */
-int __pci_read_base(struct pci_dev *dev, enum pci_bar_type type,
-		    struct resource *res, unsigned int pos)
+int __pci_bus_read_base(struct pci_bus *bus, unsigned int devfn,
+			enum pci_bar_type type,
+			struct resource *res, unsigned int pos,
+			bool mmio_always_on)
 {
 	u32 l = 0, sz = 0, mask;
 	u64 l64, sz64, mask64;
@@ -185,20 +190,18 @@ int __pci_read_base(struct pci_dev *dev, enum pci_bar_type type,
 	mask = type ? PCI_ROM_ADDRESS_MASK : ~0;
 
 	/* No printks while decoding is disabled! */
-	if (!dev->mmio_always_on) {
-		pci_read_config_word(dev, PCI_COMMAND, &orig_cmd);
+	if (!mmio_always_on) {
+		pci_bus_read_config_word(bus, devfn, PCI_COMMAND, &orig_cmd);
 		if (orig_cmd & PCI_COMMAND_DECODE_ENABLE) {
-			pci_write_config_word(dev, PCI_COMMAND,
+			pci_bus_write_config_word(bus, devfn, PCI_COMMAND,
 				orig_cmd & ~PCI_COMMAND_DECODE_ENABLE);
 		}
 	}
 
-	res->name = pci_name(dev);
-
-	pci_read_config_dword(dev, pos, &l);
-	pci_write_config_dword(dev, pos, l | mask);
-	pci_read_config_dword(dev, pos, &sz);
-	pci_write_config_dword(dev, pos, l);
+	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.
@@ -217,7 +220,7 @@ int __pci_read_base(struct pci_dev *dev, enum pci_bar_type type,
 		l = 0;
 
 	if (type == pci_bar_unknown) {
-		res->flags = decode_bar(dev, l);
+		res->flags = decode_bar(l);
 		res->flags |= IORESOURCE_SIZEALIGN;
 		if (res->flags & IORESOURCE_IO) {
 			l64 = l & PCI_BASE_ADDRESS_IO_MASK;
@@ -237,26 +240,25 @@ int __pci_read_base(struct pci_dev *dev, enum pci_bar_type type,
 	}
 
 	if (res->flags & IORESOURCE_MEM_64) {
-		pci_read_config_dword(dev, pos + 4, &l);
-		pci_write_config_dword(dev, pos + 4, ~0);
-		pci_read_config_dword(dev, pos + 4, &sz);
-		pci_write_config_dword(dev, pos + 4, l);
+		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 (!dev->mmio_always_on && (orig_cmd & PCI_COMMAND_DECODE_ENABLE))
-		pci_write_config_word(dev, PCI_COMMAND, orig_cmd);
+	if (!mmio_always_on && (orig_cmd & PCI_COMMAND_DECODE_ENABLE))
+		pci_bus_write_config_word(bus, devfn, PCI_COMMAND, orig_cmd);
 
 	if (!sz64)
 		goto fail;
 
 	sz64 = pci_size(l64, sz64, mask64);
 	if (!sz64) {
-		pci_info(dev, FW_BUG "reg 0x%x: invalid BAR (can't size)\n",
-			 pos);
+		pci_bus_info(bus, devfn, FW_BUG "reg 0x%x: invalid BAR (can't size)\n", pos);
 		goto fail;
 	}
 
@@ -266,8 +268,9 @@ int __pci_read_base(struct pci_dev *dev, enum pci_bar_type type,
 			res->flags |= IORESOURCE_UNSET | IORESOURCE_DISABLED;
 			res->start = 0;
 			res->end = 0;
-			pci_err(dev, "reg 0x%x: can't handle BAR larger than 4GB (size %#010llx)\n",
-				pos, (unsigned long long)sz64);
+			pci_bus_err(bus, devfn,
+				    "reg 0x%x: can't handle BAR larger than 4GB (size %#010llx)\n",
+				    pos, (unsigned long long)sz64);
 			goto out;
 		}
 
@@ -276,8 +279,9 @@ int __pci_read_base(struct pci_dev *dev, enum pci_bar_type type,
 			res->flags |= IORESOURCE_UNSET;
 			res->start = 0;
 			res->end = sz64 - 1;
-			pci_info(dev, "reg 0x%x: can't handle BAR above 4GB (bus address %#010llx)\n",
-				 pos, (unsigned long long)l64);
+			pci_bus_info(bus, devfn,
+				     "reg 0x%x: can't handle BAR above 4GB (bus address %#010llx)\n",
+				     pos, (unsigned long long)l64);
 			goto out;
 		}
 	}
@@ -285,8 +289,8 @@ int __pci_read_base(struct pci_dev *dev, enum pci_bar_type type,
 	region.start = l64;
 	region.end = l64 + sz64 - 1;
 
-	pcibios_bus_to_resource(dev->bus, res, &region);
-	pcibios_resource_to_bus(dev->bus, &inverted_region, res);
+	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
@@ -303,18 +307,17 @@ int __pci_read_base(struct pci_dev *dev, enum pci_bar_type type,
 		res->flags |= IORESOURCE_UNSET;
 		res->start = 0;
 		res->end = region.end - region.start;
-		pci_info(dev, "reg 0x%x: initial BAR value %#010llx invalid\n",
-			 pos, (unsigned long long)region.start);
+		pci_bus_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)
-		pci_info(dev, "reg 0x%x: %pR\n", pos, res);
+		pci_bus_info(bus, devfn, "reg 0x%x: %pR\n", pos, res);
 
 	return (res->flags & IORESOURCE_MEM_64) ? 1 : 0;
 }
-- 
2.34.1

[PATCH v3 3/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 it's 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 part of the
address space 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.

  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 though. In order to prevent OS from
the assignment to 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 unhinde 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 IFWI code (including BIOS) and
   already known to be problematic in the past for end users

 - the PCI by it's 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 to 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://lab.whitequark.org/files/gpioke/Intel-332690-004EN.pdf
[3]: https://lab.whitequark.org/files/gpioke/Intel-332691-002EN.pdf
[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>
---
 drivers/platform/x86/intel/Kconfig     | 12 ++++
 drivers/platform/x86/intel/Makefile    |  2 +
 drivers/platform/x86/intel/p2sb.c      | 93 ++++++++++++++++++++++++++
 include/linux/platform_data/x86/p2sb.h | 27 ++++++++
 4 files changed, 134 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 38ce3e344589..e0cc64dcf72c 100644
--- a/drivers/platform/x86/intel/Kconfig
+++ b/drivers/platform/x86/intel/Kconfig
@@ -81,6 +81,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 7c24be2423d8..b1f74b3f9c29 100644
--- a/drivers/platform/x86/intel/Makefile
+++ b/drivers/platform/x86/intel/Makefile
@@ -26,6 +26,8 @@ intel_int0002_vgpio-y			:= int0002_vgpio.o
 obj-$(CONFIG_INTEL_INT0002_VGPIO)	+= intel_int0002_vgpio.o
 intel_oaktrail-y			:= oaktrail.o
 obj-$(CONFIG_INTEL_OAKTRAIL)		+= intel_oaktrail.o
+intel_p2sb-y				:= p2sb.o
+obj-$(CONFIG_P2SB)			+= intel_p2sb.o
 
 # Intel PMIC / PMC / P-Unit drivers
 intel_bxtwc_tmu-y			:= bxtwc_tmu.o
diff --git a/drivers/platform/x86/intel/p2sb.c b/drivers/platform/x86/intel/p2sb.c
new file mode 100644
index 000000000000..b47517572310
--- /dev/null
+++ b/drivers/platform/x86/intel/p2sb.c
@@ -0,0 +1,93 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Primary to Sideband (P2SB) bridge access support
+ *
+ * Copyright (c) 2017, 2021 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>
+
+/* For __pci_bus_read_base(), which is available for the PCI subsystem */
+#include <../../../pci/pci.h>
+
+#include <asm/cpu_device_id.h>
+#include <asm/intel-family.h>
+
+#define P2SBC_HIDE_BYTE			0xe1
+#define P2SBC_HIDE_BIT			BIT(0)
+
+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;
+}
+
+/**
+ * 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;
+	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 */
+	pci_bus_write_config_byte(bus, devfn_p2sb, P2SBC_HIDE_BYTE, 0);
+
+	/* Read the first BAR of the device in question */
+	__pci_bus_read_base(bus, devfn, pci_bar_unknown, mem, PCI_BASE_ADDRESS_0, true);
+
+	/* Hide the P2SB device */
+	pci_bus_write_config_byte(bus, devfn_p2sb, P2SBC_HIDE_BYTE, P2SBC_HIDE_BIT);
+
+	pci_unlock_rescan_remove();
+
+	pci_bus_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 v3 3/8] platform/x86/intel: Add Primary to Sideband (P2SB) bridge support

[Bjorn Helgaas]

On Tue, Dec 21, 2021 at 08:15:21PM +0200, 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.

The fact that BIOS hid the BAR is a good hint that BIOS doesn't want
us to touch it.

> 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 it's nature and hardware design.

s/it's/its/

> 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 part of the
> address space which is represented by the only 64-bit BAR0. It means
> that OS mustn't reallocate the BAR.

You say the "least significant part of the *address space*" -- I don't
know what that would be, unless you mean the least-significant bits of
a *BAR*.

The general rule is that the OS is allowed to reassign BARs unless the
firmware tells us otherwise via the "Preserve PCI Boot Configuration"
_DSM.

I'm not familiar with the rule that the least-significant bits of a
BAR being hardwired to zero means the OS must not reallocate the BAR.
Per spec, the least-significant bits being hardwired to zero is what
tells us the *size* of 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.

This doesn't sound interesting; it sounds like standard PCI behavior.
Per PCIe r5.0, sec 7.5.1.1.9, "When [Multi-Function Device is] Clear,
software must not probe for Functions other than Function 0 unless
explicitly indicated by another mechanism, such as an ARI or SR-IOV
Extended Capability structure."

So software can't probe for functions other than 0 unless function 0
is present and has the Multi-Function Device bit set.

Is this P2SB thing function 0?

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

Are you saying it works like this?

  pci_read_config_word  (p2sb_dev, PCI_VENDOR_ID, &id);	# id = 0x8086
  pci_write_config_dword(p2sb_dev, 0xe0, HIDE);
  pci_read_config_word  (p2sb_dev, PCI_VENDOR_ID, &id);	# id = 0xffff
  pci_write_config_dword(p2sb_dev, 0xe0, ~HIDE);
  pci_read_config_word  (p2sb_dev, PCI_VENDOR_ID, &id);	# id = 0x8086

> This doesn't prevent MMIO accesses though. In order to prevent OS from
> the assignment to 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:

"In order to prevent OS from the assignment to these addresses"
doesn't read quite right.  Is it supposed to say something about
"preventing the OS from assigning these addresses"?

When assigning space to PCI devices, the OS is only allowed to use
space from the host bridge _CRS.

Is the above supposed to say that the firmware omits a region from the
host bridge _CRS to prevent the OS from using it?  That's the standard
behavior, of course.

And, of course, if the OS cannot enumerate a PCI device, obviously it
cannot reassign any BARs on this device it doesn't know about.

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

Ah, OK, maybe this is seeping in.  Tell me if I'm understanding
correctly:

  - P2SB is a device connected via PCI that has one BAR
  - Firmware programs the BAR to [mem 0xd0000000-0xd0ffffff]
  - Firmware sets the P2SB HIDE bit
  - P2SB now returns ~0 to config reads, handles config writes
    normally, and handles MMIO reads/writes normally
  - Firmware ensures [mem 0xd0000000-0xd0ffffff] is not available to
    the OS by removing it from _CRS and marking it "RESERVED" in e820
    or EFI memory map
  - P2SB returns ~0 to subsequent config reads
  - Therefore, OS cannot enumerate P2SB

Now you want to know the BAR value (0xd0000000) so you can do
something with it, so you clear the HIDE bit, read the BAR,
and set the HIDE bit again.

> 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 unhinde 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:

s/unhinde/unhide/

>  - 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 IFWI code (including BIOS) and
>    already known to be problematic in the past for end users

No clue what "IFWI" means.

The hardware and BIOS went to some trouble to hide this MMIO space
from the OS, but now the OS wants to use it.  I'm pretty sure the
systems were never tested against *that* configuration either :)

And the fact that they went to all this trouble to hide it means
the BIOS is likely using it for its own purposes and the OS may cause
conflicts if it also uses it.

The way the BIOS has this set up, P2SB is logically not a PCI device.
It is not enumerable.  The MMIO space it uses is not in the _CRS of a
PCI host bridge.  That means it's now a platform device.

Hopefully P2SB is not behind a PCI-to-PCI bridge that *is* under OS
control, because the OS might change the bridge apertures so the MMIO
space is no longer reachable.

The correct way to use this would be as an ACPI device so the OS can
enumerate it and the firmware can mediate access to it.  Going behind
the back of the firmware does not sound advisable to me.

If you want to hack something in, I think it might be easier to treat
this purely as a platform device instead of a PCI device.  You can
hack up the config accesses you need, discover the MMIO address, plug
that in as a resource of the platform device, and go wild.  I don't
think the PCI core needs to be involved at all.

>  - the PCI by it's 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

s/it's/its/

>  - 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 to many
> times to take static information that may be saved once per boot.

s/to many/too many/ or even better, s/to many/many/

> Links
> =====
> [1]: https://lab.whitequark.org/notes/2017-11-08/accessing-intel-ich-pch-gpios/

Reverse engineering notes?  Nice, but not really what I would expect
to see in patches coming from INTEL to enable some INTEL hardware.

> [2]: https://lab.whitequark.org/files/gpioke/Intel-332690-004EN.pdf

An Intel datasheet (good) but from a non-Intel site (not so good).

> [3]: https://lab.whitequark.org/files/gpioke/Intel-332691-002EN.pdf

Again?  Use an intel.com link if you can.

If these support something in the commit log above, can you make the
connection a little clearer?  I guess one of these has a section
3.1.39?

> [4]: https://medium.com/@jacksonchen_43335/bios-gpio-p2sb-70e9b829b403

Are these notes on reverse engineering this thing?  Doesn't really
seem like useful information in this one.

Bjorn

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

[Andy Shevchenko]

On Thu, Jan 06, 2022 at 07:03:05PM -0600, Bjorn Helgaas wrote:
> On Tue, Dec 21, 2021 at 08:15:21PM +0200, Andy Shevchenko wrote:

Thanks for review, my answers below.

> > 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.
> 
> The fact that BIOS hid the BAR is a good hint that BIOS doesn't want
> us to touch it.

The main reason is to avoid yellow bang in Windows. But the consequences of
this are the untested configurations in case it's always enabled and unhidden.

> > 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 it's nature and hardware design.
> 
> s/it's/its/

Fixed locally.

> > 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 part of the
> > address space which is represented by the only 64-bit BAR0. It means
> > that OS mustn't reallocate the BAR.
> 
> You say the "least significant part of the *address space*" -- I don't
> know what that would be, unless you mean the least-significant bits of
> a *BAR*.

Yeah, I rephrased this as

  "...the least significant bits of the base address register which is
  represented by the only 64-bit BAR0."

> The general rule is that the OS is allowed to reassign BARs unless the
> firmware tells us otherwise via the "Preserve PCI Boot Configuration"
> _DSM.
> 
> I'm not familiar with the rule that the least-significant bits of a
> BAR being hardwired to zero means the OS must not reallocate the BAR.
> Per spec, the least-significant bits being hardwired to zero is what
> tells us the *size* of the BAR.

I believe more about this is discussed below (1).

> > 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.
> 
> This doesn't sound interesting; it sounds like standard PCI behavior.
> Per PCIe r5.0, sec 7.5.1.1.9, "When [Multi-Function Device is] Clear,
> software must not probe for Functions other than Function 0 unless
> explicitly indicated by another mechanism, such as an ARI or SR-IOV
> Extended Capability structure."
> 
> So software can't probe for functions other than 0 unless function 0
> is present and has the Multi-Function Device bit set.

So, you are repeating what I told you when you asked me why we read a BAR
of another device. I put it here to avoid again the same question from you
or anyone else (who may well be not familiar with the spec).

What do you want me to do here?

> Is this P2SB thing function 0?

It depends.

> > 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.
> > 
> >   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.
> 
> Are you saying it works like this?
> 
>   pci_read_config_word  (p2sb_dev, PCI_VENDOR_ID, &id);	# id = 0x8086
>   pci_write_config_dword(p2sb_dev, 0xe0, HIDE);
>   pci_read_config_word  (p2sb_dev, PCI_VENDOR_ID, &id);	# id = 0xffff
>   pci_write_config_dword(p2sb_dev, 0xe0, ~HIDE);
>   pci_read_config_word  (p2sb_dev, PCI_VENDOR_ID, &id);	# id = 0x8086

It's not me, the documentation, but yes, something like you provided above
is what it does.

> > This doesn't prevent MMIO accesses though. In order to prevent OS from
> > the assignment to 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:
> 
> "In order to prevent OS from the assignment to these addresses"
> doesn't read quite right.  Is it supposed to say something about
> "preventing the OS from assigning these addresses"?

Yes, thanks, I fixed as suggested.

> When assigning space to PCI devices, the OS is only allowed to use
> space from the host bridge _CRS.

(1)

Correct and the P2SB area is not included there.

> Is the above supposed to say that the firmware omits a region from the
> host bridge _CRS to prevent the OS from using it?  That's the standard
> behavior, of course.

Yes, and again the purpose of this paragraph is to document the background
of all these as requested by you in previous round. It might be I misread
what you wanted that time.

> And, of course, if the OS cannot enumerate a PCI device, obviously it
> cannot reassign any BARs on this device it doesn't know about.

Exactly. And I believe this explains why the region is excluded from _CRS and
why we mustn't reallocate it. It probably will work (again, no-one have broadly
tested this), but it will consume resources which can be used by others (like
Thunderbolt).

> >   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.
> 
> Ah, OK, maybe this is seeping in.  Tell me if I'm understanding
> correctly:
> 
>   - P2SB is a device connected via PCI that has one BAR

Yes.

>   - Firmware programs the BAR to [mem 0xd0000000-0xd0ffffff]

Yes.

>   - Firmware sets the P2SB HIDE bit

Yes.

>   - P2SB now returns ~0 to config reads,

Yes.

> handles config writes normally,

Only to 0xe0. The rest is skipped.

> and handles MMIO reads/writes normally

Yes.

>   - Firmware ensures [mem 0xd0000000-0xd0ffffff] is not available to
>     the OS by removing it from _CRS and marking it "RESERVED" in e820
>     or EFI memory map

Yes.

>   - P2SB returns ~0 to subsequent config reads
>   - Therefore, OS cannot enumerate P2SB

Correct.

> Now you want to know the BAR value (0xd0000000) so you can do
> something with it, so you clear the HIDE bit, read the BAR,
> and set the HIDE bit again.

Correct.

> > 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 unhinde 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:
> 
> s/unhinde/unhide/

Fixed locally.

> >  - 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 IFWI code (including BIOS) and
> >    already known to be problematic in the past for end users
> 
> No clue what "IFWI" means.

Intel FirmWare Image, I will spell it in full.

> The hardware and BIOS went to some trouble to hide this MMIO space
> from the OS, but now the OS wants to use it.  I'm pretty sure the
> systems were never tested against *that* configuration either :)

What do you mean? The unhide/hide back has been tested and we have
already users in the kernel (they have other issues though with the
PCI rescan lock, but it doesn't mean it wasn't ever tested).

> And the fact that they went to all this trouble to hide it means
> the BIOS is likely using it for its own purposes and the OS may cause
> conflicts if it also uses it.

What purposes do you have in mind?

> The way the BIOS has this set up, P2SB is logically not a PCI device.
> It is not enumerable.  The MMIO space it uses is not in the _CRS of a
> PCI host bridge.  That means it's now a platform device.

I do not follow what you are implying here.

As you see the code, it's not a driver, it's a library that reuses PCI
functions because the hardware is represented by an IP inside PCI hierarchy
and with PCI programming interface.

> Hopefully P2SB is not behind a PCI-to-PCI bridge that *is* under OS
> control, because the OS might change the bridge apertures so the MMIO
> space is no longer reachable.

No it's not (at least on all platforms what I know of). We are good here.

> The correct way to use this would be as an ACPI device so the OS can
> enumerate it and the firmware can mediate access to it.  Going behind
> the back of the firmware does not sound advisable to me.

Are you going to fix all firmwares and devices on the market?
We have it's done like this and unfortunately we can't fix what's
is done due to users who won't update their firmwares by one or
another reason.

> If you want to hack something in, I think it might be easier to treat
> this purely as a platform device instead of a PCI device.  You can
> hack up the config accesses you need, discover the MMIO address, plug
> that in as a resource of the platform device, and go wild.  I don't
> think the PCI core needs to be involved at all.

Sorry, I do not follow you. The device is PCI, but it's taken out of PCI
subsystem control by this hardware trick.

> >  - the PCI by it's 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

> s/it's/its/

Fixed locally.

> >  - 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 to many
> > times to take static information that may be saved once per boot.
> 
> s/to many/too many/ or even better, s/to many/many/

Fixed locally.

> > Links
> > =====
> > [1]: https://lab.whitequark.org/notes/2017-11-08/accessing-intel-ich-pch-gpios/
> 
> Reverse engineering notes?  Nice, but not really what I would expect
> to see in patches coming from INTEL to enable some INTEL hardware.
> 
> > [2]: https://lab.whitequark.org/files/gpioke/Intel-332690-004EN.pdf
> 
> An Intel datasheet (good) but from a non-Intel site (not so good).
> 
> > [3]: https://lab.whitequark.org/files/gpioke/Intel-332691-002EN.pdf
> 
> Again?  Use an intel.com link if you can.

There are document numbers that make sense.
I believe that

[2]: https://cdrdv2.intel.com/v1/dl/getContent/332690?wapkw=332690
[3]: https://cdrdv2.intel.com/v1/dl/getContent/332691?wapkw=332691

work for you. Tell me if not (Meanwhile I have changed locally)

> If these support something in the commit log above, can you make the
> connection a little clearer?  I guess one of these has a section
> 3.1.39?

Done.

> > [4]: https://medium.com/@jacksonchen_43335/bios-gpio-p2sb-70e9b829b403
> 
> Are these notes on reverse engineering this thing?  Doesn't really
> seem like useful information in this one.

-- 
With Best Regards,
Andy Shevchenko

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

[Bjorn Helgaas]

On Fri, Jan 07, 2022 at 04:56:42PM +0200, Andy Shevchenko wrote:
> On Thu, Jan 06, 2022 at 07:03:05PM -0600, Bjorn Helgaas wrote:
> > On Tue, Dec 21, 2021 at 08:15:21PM +0200, Andy Shevchenko wrote:

> > And, of course, if the OS cannot enumerate a PCI device, obviously
> > it cannot reassign any BARs on this device it doesn't know about.
> 
> Exactly. And I believe this explains why the region is excluded from
> _CRS and why we mustn't reallocate it. It probably will work (again,
> no-one have broadly tested this), but it will consume resources
> which can be used by others (like Thunderbolt).

Firmware excluded it from _CRS because it knows the region is in use.
If it *were* in _CRS, the OS would see that no device uses it, so it
could assign it to some PCI device, and an MMIO read would get two
responses.

If you manually reassign that BAR to some unused address space
elsewhere, we have no idea what would happen.  Since the device is not
described anywhere, we have no idea what address space even *reaches*
the device.  And if firmware is using the device, changing the BAR
will likely break whatever firmware is doing.

> > The hardware and BIOS went to some trouble to hide this MMIO space
> > from the OS, but now the OS wants to use it.  I'm pretty sure the
> > systems were never tested against *that* configuration either :)
> 
> What do you mean? The unhide/hide back has been tested and we have
> already users in the kernel (they have other issues though with the
> PCI rescan lock, but it doesn't mean it wasn't ever tested).

Does the firmware team that hid this device sign off on the OS
unhiding and using it?  How do we know that BIOS is not using the
device?

> > And the fact that they went to all this trouble to hide it means
> > the BIOS is likely using it for its own purposes and the OS may
> > cause conflicts if it also uses it.
> 
> What purposes do you have in mind?

The functionality implemented in the P2SB MMIO space is not specified,
so I have no idea what it does or whether BIOS could be using it.

But here's a hypothetical example: some platform firmware logs errors
to NVRAM.  That NVRAM could exist on a device like the P2SB, where the
firmware assigns the MMIO address and hides the device from the OS.
The firmware legitimately assumes it has exclusive control of the
device and the OS will never touch it.  If the OS unhides the device
and also uses that NVRAM, the platform error logging no longer works.

My point is that the unhide is architecturally messed up.  The OS runs
on the platform as described by ACPI.  Devices that cannot be
enumerated are described in the ACPI namespace.

If the OS goes outside that ACPI-described platform and pokes at
things it "knows" should be there, the architectural model falls
apart.  The OS relies on things the firmware didn't guarantee, and
the firmware can't rely on non-interference from the OS.

If you want to go outside the ACPI model, that's up to you, but I
don't think we should tweak the PCI core to work with things that
the BIOS has explicitly arranged to *not* be PCI devices.

> > The way the BIOS has this set up, P2SB is logically not a PCI
> > device.  It is not enumerable.  The MMIO space it uses is not in
> > the _CRS of a PCI host bridge.  That means it's now a platform
> > device.
> 
> I do not follow what you are implying here.

On an ACPI system, the way we enumerate PCI devices is to find all the
PCI host bridges (ACPI PNP0A03 devices), and scan config space to find
the PCI devices below them.  That doesn't find P2SB, so from a
software point of view, it is not a PCI device.

Platform devices are by definition non-enumerable, and they have to be
described via ACPI, DT, or some kind of platform-specific code.  P2SB
is not enumerable, so I think a platform device is the most natural
way to handle it.

> As you see the code, it's not a driver, it's a library that reuses
> PCI functions because the hardware is represented by an IP inside
> PCI hierarchy and with PCI programming interface.

Yes, it's a PCI programming interface at the hardware level, but at
the software level, it's not part of PCI.

This series does quite a lot of work in the PCI core to read that one
register in a device the PCI core doesn't know about.  I think it will
be simpler overall if instead of wedging this into PCI, we make p2sb.c
start with the ECAM base, ioremap() it, compute the register address,
readl() the MMIO address, and be done with it.  No need to deal with
pci_find_bus(), pci_lock_rescan_remove(), change the core's BAR sizing
code, etc.

> > The correct way to use this would be as an ACPI device so the OS
> > can enumerate it and the firmware can mediate access to it.  Going
> > behind the back of the firmware does not sound advisable to me.
> 
> Are you going to fix all firmwares and devices on the market?  We
> have it's done like this and unfortunately we can't fix what's is
> done due to users who won't update their firmwares by one or another
> reason.

I just mean that from a platform design standpoint, an ACPI device
would be the right way to do this.  Obviously it's not practical to
add that to systems in the field.  You could create a platform_device
manually now, and if there ever is an ACPI device, ACPI can create a
platform_device for you.

> > If you want to hack something in, I think it might be easier to
> > treat this purely as a platform device instead of a PCI device.
> > You can hack up the config accesses you need, discover the MMIO
> > address, plug that in as a resource of the platform device, and go
> > wild.  I don't think the PCI core needs to be involved at all.
> 
> Sorry, I do not follow you. The device is PCI, but it's taken out of
> PCI subsystem control by this hardware trick.

The electrical connection might be PCI, but from the software point of
view, it's only a PCI device if it can be enumerated by the mechanism
specified by the spec, namely, reading the Vendor ID of each potential
device.

Yes, doing it as a platform device would involve some code in p2sb.c
that looks sort of like code in the PCI core.  But I don't think it's
actually very much, and I think it would be less confusing than trying
to pretend that this device sometimes behaves like a PCI device and
sometimes not.

> There are document numbers that make sense.
> I believe that
> 
> [2]: https://cdrdv2.intel.com/v1/dl/getContent/332690?wapkw=332690
> [3]: https://cdrdv2.intel.com/v1/dl/getContent/332691?wapkw=332691
> 
> work for you. Tell me if not (Meanwhile I have changed locally)

Great, thanks.  The links work for me (currently).  I think a proper
citation would also include the document title and document number,
since I doubt Intel guarantees those URLs will work forever.

Bjorn

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

[Andy Shevchenko]

On Fri, Jan 07, 2022 at 11:11:08AM -0600, Bjorn Helgaas wrote:
> On Fri, Jan 07, 2022 at 04:56:42PM +0200, Andy Shevchenko wrote:
> > On Thu, Jan 06, 2022 at 07:03:05PM -0600, Bjorn Helgaas wrote:
> > > On Tue, Dec 21, 2021 at 08:15:21PM +0200, Andy Shevchenko wrote:

...

> > The unhide/hide back has been tested and we have
> > already users in the kernel (they have other issues though with the
> > PCI rescan lock, but it doesn't mean it wasn't ever tested).
> 
> Does the firmware team that hid this device sign off on the OS
> unhiding and using it?  How do we know that BIOS is not using the
> device?

BIOS might use the device via OperationRegion() in ACPI, but that means
that _CRS needs to have that region available. It seems not the case.

And as far I as see in the internal documentation the hide / unhide
approach is not forbidden for OS side.

Moreover, we have already this approach in the 3 device drivers on different
platforms. If you not agree with it, probably you can send a removal to that
drivers. In the terms of use this code doesn't change the status quo. What
it does is the concentration of the p2sb code in one place as a library on
obvious (?) purposes, e.g. maintenance.

> > > And the fact that they went to all this trouble to hide it means
> > > the BIOS is likely using it for its own purposes and the OS may
> > > cause conflicts if it also uses it.
> > 
> > What purposes do you have in mind?
> 
> The functionality implemented in the P2SB MMIO space is not specified,
> so I have no idea what it does or whether BIOS could be using it.

It's specified based on how MMIO address is encoded.

The third byte (bits [23:16]) representing the port ID on IOSF that
belongs to the certain IPs, such as GPIO.

> But here's a hypothetical example: some platform firmware logs errors
> to NVRAM.  That NVRAM could exist on a device like the P2SB, where the
> firmware assigns the MMIO address and hides the device from the OS.
> The firmware legitimately assumes it has exclusive control of the
> device and the OS will never touch it.  If the OS unhides the device
> and also uses that NVRAM, the platform error logging no longer works.
> 
> My point is that the unhide is architecturally messed up.  The OS runs
> on the platform as described by ACPI.  Devices that cannot be
> enumerated are described in the ACPI namespace.

This device may or may not be _partially_ or _fully_ (due to being
multifunctional) described in ACPI. I agree, that ideally the devices
in question it has behind should be represented properly by firmware.
However, the firmwares in the wild for selected products / devices
don't do that. We need to solve (work around) it in the software.

This is already done for a few devices. This series consolidates that
and enables it for very known GPIO IPs.

> If the OS goes outside that ACPI-described platform and pokes at
> things it "knows" should be there, the architectural model falls
> apart.  The OS relies on things the firmware didn't guarantee, and
> the firmware can't rely on non-interference from the OS.
> 
> If you want to go outside the ACPI model, that's up to you, but I
> don't think we should tweak the PCI core to work with things that
> the BIOS has explicitly arranged to *not* be PCI devices.

PCI core just provides a code that is very similar to what we need
here. Are you specifically suggesting that we have to copy'n'paste
that rather long function and maintain in parallel with PCI?

> > > The way the BIOS has this set up, P2SB is logically not a PCI
> > > device.  It is not enumerable.  The MMIO space it uses is not in
> > > the _CRS of a PCI host bridge.  That means it's now a platform
> > > device.
> > 
> > I do not follow what you are implying here.
> 
> On an ACPI system, the way we enumerate PCI devices is to find all the
> PCI host bridges (ACPI PNP0A03 devices), and scan config space to find
> the PCI devices below them.  That doesn't find P2SB, so from a
> software point of view, it is not a PCI device.

It's a PCI device that has a PCI programming interface but it has some
tricks behind. Do you mean that those tricks automatically make it non-PCI
(software speaking) compatible?

> Platform devices are by definition non-enumerable, and they have to be
> described via ACPI, DT, or some kind of platform-specific code.  P2SB
> is not enumerable, so I think a platform device is the most natural
> way to handle it.

How does it fit the proposed library model? Are you suggesting to create a
hundreds of LOCs in order just to have some platform device which does what?

I do not follow here the design you are proposing, sorry.

> > As you see the code, it's not a driver, it's a library that reuses
> > PCI functions because the hardware is represented by an IP inside
> > PCI hierarchy and with PCI programming interface.
> 
> Yes, it's a PCI programming interface at the hardware level, but at
> the software level, it's not part of PCI.

Why?

> This series does quite a lot of work in the PCI core to read that one
> register in a device the PCI core doesn't know about.  I think it will
> be simpler overall if instead of wedging this into PCI, we make p2sb.c
> start with the ECAM base, ioremap() it, compute the register address,
> readl() the MMIO address, and be done with it.  No need to deal with
> pci_find_bus(), pci_lock_rescan_remove(), change the core's BAR sizing
> code, etc.

So, you are suggesting to write a (simplified) PCI core for the certain device,
did I get you right? Would it have good long-term maintenance perspective?

> > > The correct way to use this would be as an ACPI device so the OS
> > > can enumerate it and the firmware can mediate access to it.  Going
> > > behind the back of the firmware does not sound advisable to me.
> > 
> > Are you going to fix all firmwares and devices on the market?  We
> > have it's done like this and unfortunately we can't fix what's is
> > done due to users who won't update their firmwares by one or another
> > reason.
> 
> I just mean that from a platform design standpoint, an ACPI device
> would be the right way to do this.  Obviously it's not practical to
> add that to systems in the field.  You could create a platform_device
> manually now, and if there ever is an ACPI device, ACPI can create a
> platform_device for you.

Why do I need that device? What for? I really don't see a point here.

> > > If you want to hack something in, I think it might be easier to
> > > treat this purely as a platform device instead of a PCI device.
> > > You can hack up the config accesses you need, discover the MMIO
> > > address, plug that in as a resource of the platform device, and go
> > > wild.  I don't think the PCI core needs to be involved at all.
> > 
> > Sorry, I do not follow you. The device is PCI, but it's taken out of
> > PCI subsystem control by this hardware trick.
> 
> The electrical connection might be PCI, but from the software point of
> view, it's only a PCI device if it can be enumerated by the mechanism
> specified by the spec, namely, reading the Vendor ID of each potential
> device.
> 
> Yes, doing it as a platform device would involve some code in p2sb.c
> that looks sort of like code in the PCI core.  But I don't think it's
> actually very much, and I think it would be less confusing than trying
> to pretend that this device sometimes behaves like a PCI device and
> sometimes not.

So, duplicating code is good, right? Why do we have libraries in the code?

> > There are document numbers that make sense.
> > I believe that
> > 
> > [2]: https://cdrdv2.intel.com/v1/dl/getContent/332690?wapkw=332690
> > [3]: https://cdrdv2.intel.com/v1/dl/getContent/332691?wapkw=332691
> > 
> > work for you. Tell me if not (Meanwhile I have changed locally)
> 
> Great, thanks.  The links work for me (currently).  I think a proper
> citation would also include the document title and document number,
> since I doubt Intel guarantees those URLs will work forever.

-- 
With Best Regards,
Andy Shevchenko

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

[Bjorn Helgaas]

On Fri, Jan 28, 2022 at 08:30:20PM +0200, Andy Shevchenko wrote:
> On Fri, Jan 07, 2022 at 11:11:08AM -0600, Bjorn Helgaas wrote:
> > On Fri, Jan 07, 2022 at 04:56:42PM +0200, Andy Shevchenko wrote:
> 
> ...
> 
> > > The unhide/hide back has been tested and we have already users
> > > in the kernel (they have other issues though with the PCI rescan
> > > lock, but it doesn't mean it wasn't ever tested).
> > 
> > Does the firmware team that hid this device sign off on the OS
> > unhiding and using it?  How do we know that BIOS is not using the
> > device?
> 
> BIOS might use the device via OperationRegion() in ACPI, but that
> means that _CRS needs to have that region available. It seems not
> the case.
> 
> And as far I as see in the internal documentation the hide / unhide
> approach is not forbidden for OS side.

Unhiding is device-specific behavior, so generic PCI enumeration
cannot use it.  We have to know there's a P2SB device at some address
before we can safely do a config write to it.  PCI enumeration would
learn there's a P2SB device at an address by reading a Vendor/Device
ID.

> > My point is that the unhide is architecturally messed up.  The OS
> > runs on the platform as described by ACPI.  Devices that cannot be
> > enumerated are described in the ACPI namespace.
> 
> This device may or may not be _partially_ or _fully_ (due to being
> multifunctional) described in ACPI. I agree, that ideally the
> devices in question it has behind should be represented properly by
> firmware.  However, the firmwares in the wild for selected products
> / devices don't do that. We need to solve (work around) it in the
> software.
> 
> This is already done for a few devices. This series consolidates
> that and enables it for very known GPIO IPs.

Consolidating the code to unhide the device and size the BAR is fine.

I would prefer the PCI core to be involved as little as possible
because we're violating some key assumptions and we could trip over
those later.  We're assuming the existence of P2SB based on the fact
that we found some *other* device, we're assuming firmware isn't using
P2SB (may be true now, but impossible to verify), we're assuming the
P2SB BAR contains a valid address that's not used elsewhere but also
won't be assigned to anything.

> PCI core just provides a code that is very similar to what we need
> here. Are you specifically suggesting that we have to copy'n'paste
> that rather long function and maintain in parallel with PCI?

I think we're talking about __pci_read_base(), which is currently an
internal PCI interface.  This series adds pci_bus_info/warn/etc(),
reworks __pci_read_base() to operate on a struct pci_bus *, exports
it, and uses it via #include <../../../pci/pci.h>.

__pci_read_base() is fairly long, but you apparently don't need all
the functionality there because the core of the patch is this:

  -   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;
  -   }
  +   __pci_bus_read_base(bus, devfn, pci_bar_unknown, mem, PCI_BASE_ADDRESS_0, true)

I don't think it's worth all the __pci_read_base() changes to do that.
What if you made a library function that looks like this?

  int p2sb_bar(...)
  {
    pci_bus_write_config_byte(bus, devfn_p2sb, P2SBC_HIDE_BYTE, 0);
    pci_bus_read_config_dword(bus, devfn, PCI_BASE_ADDRESS_0, &orig);
    if (orig) {
      pci_bus_write_config_dword(bus, devfn, PCI_BASE_ADDRESS_0, ~0);
      pci_bus_read_config_dword(bus, devfn, PCI_BASE_ADDRESS_0, &val);
      pci_bus_write_config_dword(bus, devfn, PCI_BASE_ADDRESS_0, orig);
      res->start = orig;
      res->end = res->start + (~val + 1);
    }
    pci_bus_write_config_byte(bus, devfn, P2SBC_HIDE_BYTE, P2SBC_HIDE_BIT);
  }

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

[Andy Shevchenko]

On Tue, Feb 01, 2022 at 12:14:01PM -0600, Bjorn Helgaas wrote:
> On Fri, Jan 28, 2022 at 08:30:20PM +0200, Andy Shevchenko wrote:
> > On Fri, Jan 07, 2022 at 11:11:08AM -0600, Bjorn Helgaas wrote:
> > > On Fri, Jan 07, 2022 at 04:56:42PM +0200, Andy Shevchenko wrote:
> > 
> > ...
> > 
> > > > The unhide/hide back has been tested and we have already users
> > > > in the kernel (they have other issues though with the PCI rescan
> > > > lock, but it doesn't mean it wasn't ever tested).
> > > 
> > > Does the firmware team that hid this device sign off on the OS
> > > unhiding and using it?  How do we know that BIOS is not using the
> > > device?
> > 
> > BIOS might use the device via OperationRegion() in ACPI, but that
> > means that _CRS needs to have that region available. It seems not
> > the case.
> > 
> > And as far I as see in the internal documentation the hide / unhide
> > approach is not forbidden for OS side.
> 
> Unhiding is device-specific behavior, so generic PCI enumeration
> cannot use it.  We have to know there's a P2SB device at some address
> before we can safely do a config write to it.  PCI enumeration would
> learn there's a P2SB device at an address by reading a Vendor/Device
> ID.
> 
> > > My point is that the unhide is architecturally messed up.  The OS
> > > runs on the platform as described by ACPI.  Devices that cannot be
> > > enumerated are described in the ACPI namespace.
> > 
> > This device may or may not be _partially_ or _fully_ (due to being
> > multifunctional) described in ACPI. I agree, that ideally the
> > devices in question it has behind should be represented properly by
> > firmware.  However, the firmwares in the wild for selected products
> > / devices don't do that. We need to solve (work around) it in the
> > software.
> > 
> > This is already done for a few devices. This series consolidates
> > that and enables it for very known GPIO IPs.
> 
> Consolidating the code to unhide the device and size the BAR is fine.
> 
> I would prefer the PCI core to be involved as little as possible
> because we're violating some key assumptions and we could trip over
> those later.  We're assuming the existence of P2SB based on the fact
> that we found some *other* device, we're assuming firmware isn't using
> P2SB (may be true now, but impossible to verify), we're assuming the
> P2SB BAR contains a valid address that's not used elsewhere but also
> won't be assigned to anything.
> 
> > PCI core just provides a code that is very similar to what we need
> > here. Are you specifically suggesting that we have to copy'n'paste
> > that rather long function and maintain in parallel with PCI?
> 
> I think we're talking about __pci_read_base(), which is currently an
> internal PCI interface.  This series adds pci_bus_info/warn/etc(),

The patch that adds those macros is good on its own, if you think so...
I tried to submit it separately, but it was no response, so I don't know.

> reworks __pci_read_base() to operate on a struct pci_bus *, exports
> it, and uses it via #include <../../../pci/pci.h>.

Yes, which allows at least to have the same code, doing same things to be
in one copy in one place.

> __pci_read_base() is fairly long, but you apparently don't need all
> the functionality there because the core of the patch is this:
> 
>   -   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;
>   -   }
>   +   __pci_bus_read_base(bus, devfn, pci_bar_unknown, mem, PCI_BASE_ADDRESS_0, true)

You probably took the least pleasant (to me) example, because it's buggy in a
few ways:

- it misses 64-bit handling code
- it misses PCI rescan lock (in case PCI code decides to change addresses,
  previously ones will be invalid, while other drivers may still use that
  MMIO space
- it doesn't check if (for a new version Hans suggested me to add this check as
  it's done in one out of 3 cases)

It also useful to have some messages to be printed just in cases of errors
or success in a standard (PCI core provided) way.

> I don't think it's worth all the __pci_read_base() changes to do that.
> What if you made a library function that looks like this?
> 
>   int p2sb_bar(...)
>   {
>     pci_bus_write_config_byte(bus, devfn_p2sb, P2SBC_HIDE_BYTE, 0);
>     pci_bus_read_config_dword(bus, devfn, PCI_BASE_ADDRESS_0, &orig);
>     if (orig) {
>       pci_bus_write_config_dword(bus, devfn, PCI_BASE_ADDRESS_0, ~0);
>       pci_bus_read_config_dword(bus, devfn, PCI_BASE_ADDRESS_0, &val);
>       pci_bus_write_config_dword(bus, devfn, PCI_BASE_ADDRESS_0, orig);
>       res->start = orig;
>       res->end = res->start + (~val + 1);
>     }
>     pci_bus_write_config_byte(bus, devfn, P2SBC_HIDE_BYTE, P2SBC_HIDE_BIT);
>   }

It seems simple, but with the above mentioned adjustments, it will become
closer to the size of the original __pci_read_base().

-- 
With Best Regards,
Andy Shevchenko

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

[Bjorn Helgaas]

On Tue, Feb 01, 2022 at 08:52:22PM +0200, Andy Shevchenko wrote:
> On Tue, Feb 01, 2022 at 12:14:01PM -0600, Bjorn Helgaas wrote:
> > On Fri, Jan 28, 2022 at 08:30:20PM +0200, Andy Shevchenko wrote:
> > > On Fri, Jan 07, 2022 at 11:11:08AM -0600, Bjorn Helgaas wrote:
> > > > On Fri, Jan 07, 2022 at 04:56:42PM +0200, Andy Shevchenko wrote:
> > > 
> > > ...
> > > 
> > > > My point is that the unhide is architecturally messed up.  The OS
> > > > runs on the platform as described by ACPI.  Devices that cannot be
> > > > enumerated are described in the ACPI namespace.
> > > 
> > > This device may or may not be _partially_ or _fully_ (due to being
> > > multifunctional) described in ACPI. I agree, that ideally the
> > > devices in question it has behind should be represented properly by
> > > firmware.  However, the firmwares in the wild for selected products
> > > / devices don't do that. We need to solve (work around) it in the
> > > software.
> > > 
> > > This is already done for a few devices. This series consolidates
> > > that and enables it for very known GPIO IPs.
> > 
> > Consolidating the code to unhide the device and size the BAR is fine.
> > 
> > I would prefer the PCI core to be involved as little as possible
> > because we're violating some key assumptions and we could trip over
> > those later.  We're assuming the existence of P2SB based on the fact
> > that we found some *other* device, we're assuming firmware isn't using
> > P2SB (may be true now, but impossible to verify), we're assuming the
> > P2SB BAR contains a valid address that's not used elsewhere but also
> > won't be assigned to anything.
> > 
> > > PCI core just provides a code that is very similar to what we need
> > > here. Are you specifically suggesting that we have to copy'n'paste
> > > that rather long function and maintain in parallel with PCI?
> > 
> > I think we're talking about __pci_read_base(), which is currently an
> > internal PCI interface.  This series adds pci_bus_info/warn/etc(),
> 
> The patch that adds those macros is good on its own, if you think so...
> I tried to submit it separately, but it was no response, so I don't know.

I'd rather not add pci_bus_info(), etc.  There are only a couple
places that could use it, and if we cared enough, I think those places
could avoid it by doing pci_alloc_dev() first.

What if you used pci_alloc_dev() and then called the current
__pci_read_base() on the pci_dev *?

The caller would still have the ugly #include path, but I guess you're
OK with that.

Since the P2SB BAR is not included in the host bridge _CRS, the
pcibios_bus_to_resource() done by __pci_read_base() won't work
correctly, so this only "works" on host bridges with no address
translation.  But that's also the case with your current series.
This is an example of one of the PCI core assumptions being violated,
so things can break.

Bjorn

[PATCH v3 4/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>
---
 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 85750974d182..7d8a7e7b0aef 100644
--- a/drivers/pinctrl/intel/pinctrl-intel.c
+++ b/drivers/pinctrl/intel/pinctrl-intel.c
@@ -1598,16 +1598,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];
@@ -1621,7 +1619,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

Re: [PATCH v3 4/8] pinctrl: intel: Check against matching data instead of ACPI companion

[Mika Westerberg]

On Tue, Dec 21, 2021 at 08:15:22PM +0200, Andy Shevchenko wrote:
> 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: Mika Westerberg <mika.westerberg@linux.intel.com>

[PATCH v3 5/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-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 v3 6/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-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 a21cbdf89477..be3616fe78b8 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 v3 7/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>
---
 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..563e4ed251fd 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 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 v3 7/8] mfd: lpc_ich: Add support for pinctrl in non-ACPI system

[Andy Shevchenko]

On Tue, Dec 21, 2021 at 08:15:25PM +0200, 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.

Lee, are you fine with this change? I hope I fixed all your comments.

-- 
With Best Regards,
Andy Shevchenko

[PATCH v3 8/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 pci_p2sb_bar() call.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 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 b47517572310..916318a7310b 100644
--- a/drivers/platform/x86/intel/p2sb.c
+++ b/drivers/platform/x86/intel/p2sb.c
@@ -24,6 +24,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 v3 8/8] i2c: i801: convert to use common P2SB accessor

[Andy Shevchenko]

On Tue, Dec 21, 2021 at 08:15:26PM +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 pci_p2sb_bar() call.

Wolfram, does this change makes sense to you?
Can you give your tag or comment?

-- 
With Best Regards,
Andy Shevchenko

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

[Linus Walleij]

On Tue, Dec 21, 2021 at 7:21 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:

> Please, comment on the approach and individual patches.

This approach looks reasonable to me so FWIW:
Acked-by: Linus Walleij <linus.walleij@linaro.org>
for the series.

Yours,
Linus Walleij

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

[Andy Shevchenko]

On Wed, Dec 22, 2021 at 03:48:15AM +0100, Linus Walleij wrote:
> On Tue, Dec 21, 2021 at 7:21 PM Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> 
> > Please, comment on the approach and individual patches.
> 
> This approach looks reasonable to me so FWIW:
> Acked-by: Linus Walleij <linus.walleij@linaro.org>
> for the series.

Thank you!

-- 
With Best Regards,
Andy Shevchenko

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

[Andy Shevchenko]

On Tue, Dec 21, 2021 at 08:15:18PM +0200, Andy Shevchenko wrote:
> 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 deduplicate existing users and provide
> a generic way for new comers.
> 
> 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.
> 
> Please, comment on the approach and individual patches.
> 
> 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.17-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).
> 
> (Since it's cross subsystem, the PDx86 seems the main one and
> I think it makes sense to route it throught it with immutable
> tag or branch provided for the others).
> 
> Bjorn, are you okay with this approach and the commit message in the main
> patch?
> 
> 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)

I'm going to be on vacation till 2022-01-03, I'll address comments if any
during the first week of January and I hope it can make v5.17. Hans, what
do you think?

(Meanwhile I'm expecting that my patch to fix dependencies will be applied,
 so kbuild bot won't complain anymore on them being broken)

-- 
With Best Regards,
Andy Shevchenko

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

[Hans de Goede]

Hi,

On 12/21/21 19:15, Andy Shevchenko wrote:
> 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 deduplicate existing users and provide
> a generic way for new comers.
> 
> 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.
> 
> Please, comment on the approach and individual patches.
> 
> 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.17-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).
> 
> (Since it's cross subsystem, the PDx86 seems the main one and
> I think it makes sense to route it throught it with immutable
> tag or branch provided for the others).

The series looks good to me:

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

For the series.

Not sure if this is really 5.17 material this late in the cycle though,
but lets wait and see what Bjorn and Lee have to say (patch 8/8 still
needs an ack from Lee).

I'm fine with taking this upstream through the pdx86 tree, please
prepare a pull-req for everyone involved with an immutable branch
pushed to pdx86/platform-drivers-x86.git/
based on 5.16-rc1 (if everyone is happy with merging this for 5.17) or
based on 5.17-rc1 once that is out.

Regards,

Hans

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

[Hans de Goede]

Hi,

On 12/23/21 18:00, Hans de Goede wrote:
> Hi,
> 
> On 12/21/21 19:15, Andy Shevchenko wrote:
>> 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 deduplicate existing users and provide
>> a generic way for new comers.
>>
>> 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.
>>
>> Please, comment on the approach and individual patches.
>>
>> 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.17-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).
>>
>> (Since it's cross subsystem, the PDx86 seems the main one and
>> I think it makes sense to route it throught it with immutable
>> tag or branch provided for the others).
> 
> The series looks good to me:
> 
> Acked-by: Hans de Goede <hdegoede@redhat.com>
> 
> For the series.
> 
> Not sure if this is really 5.17 material this late in the cycle though,
> but lets wait and see what Bjorn and Lee have to say (patch 8/8 still
> needs an ack from Lee).

Correction I just realized that that would be 7/8 that needs an ack from
Lee and that 8/8 needs an ack from Wolfram.

> I'm fine with taking this upstream through the pdx86 tree, please
> prepare a pull-req for everyone involved with an immutable branch
> pushed to pdx86/platform-drivers-x86.git/
> based on 5.16-rc1 (if everyone is happy with merging this for 5.17) or
> based on 5.17-rc1 once that is out.

Regards,

Hans