linux-i2c.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [rfc, PATCH v1 0/7] PCI: introduce p2sb helper
@ 2021-03-08 12:20 Andy Shevchenko
  2021-03-08 12:20 ` [PATCH v1 1/7] PCI: Introduce pci_bus_*() printing macros when device is not available Andy Shevchenko
                   ` (9 more replies)
  0 siblings, 10 replies; 54+ messages in thread
From: Andy Shevchenko @ 2021-03-08 12:20 UTC (permalink / raw)
  To: Wolfram Sang, Jean Delvare, Lee Jones, Andy Shevchenko,
	Tan Jui Nee, Jim Quinlan, Jonathan Yong, Bjorn Helgaas,
	linux-kernel, linux-i2c, linux-pci
  Cc: Jean Delvare, Peter Tyser, hdegoede, henning.schild

There are a few users and even at least one more is coming
that would like to utilize p2sb mechanisms like hide/unhide
a device from 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.

Please, comment on the approach and individual patches.

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

Andy Shevchenko (5):
  PCI: Introduce pci_bus_*() printing macros when device is not
    available
  PCI: Convert __pci_read_base() to __pci_bus_read_base()
  mfd: lpc_ich: Factor out lpc_ich_enable_spi_write()
  mfd: lpc_ich: Switch to generic pci_p2sb_bar()
  i2c: i801: convert to use common P2SB accessor

Jonathan Yong (1):
  PCI: New Primary to Sideband (P2SB) bridge support library

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 |  40 +++-------
 drivers/mfd/Kconfig           |   1 +
 drivers/mfd/lpc_ich.c         | 135 +++++++++++++++++++++++++++++-----
 drivers/pci/Kconfig           |   8 ++
 drivers/pci/Makefile          |   1 +
 drivers/pci/pci-p2sb.c        |  89 ++++++++++++++++++++++
 drivers/pci/pci.h             |  13 +++-
 drivers/pci/probe.c           |  81 ++++++++++----------
 include/linux/pci-p2sb.h      |  28 +++++++
 include/linux/pci.h           |   9 +++
 11 files changed, 313 insertions(+), 93 deletions(-)
 create mode 100644 drivers/pci/pci-p2sb.c
 create mode 100644 include/linux/pci-p2sb.h

-- 
2.30.1


^ permalink raw reply	[flat|nested] 54+ messages in thread

* [PATCH v1 1/7] PCI: Introduce pci_bus_*() printing macros when device is not available
  2021-03-08 12:20 [rfc, PATCH v1 0/7] PCI: introduce p2sb helper Andy Shevchenko
@ 2021-03-08 12:20 ` Andy Shevchenko
  2021-03-10 14:57   ` Jean Delvare
  2021-03-08 12:20 ` [PATCH v1 2/7] PCI: Convert __pci_read_base() to __pci_bus_read_base() Andy Shevchenko
                   ` (8 subsequent siblings)
  9 siblings, 1 reply; 54+ messages in thread
From: Andy Shevchenko @ 2021-03-08 12:20 UTC (permalink / raw)
  To: Wolfram Sang, Jean Delvare, Lee Jones, Andy Shevchenko,
	Tan Jui Nee, Jim Quinlan, Jonathan Yong, Bjorn Helgaas,
	linux-kernel, linux-i2c, linux-pci
  Cc: Jean Delvare, Peter Tyser, hdegoede, henning.schild

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>
---
 drivers/pci/probe.c | 12 +++---------
 include/linux/pci.h |  9 +++++++++
 2 files changed, 12 insertions(+), 9 deletions(-)

diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index 953f15abc850..7d67be52d8e5 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -2268,16 +2268,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;
@@ -2287,9 +2283,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 86c799c97b77..c557d4a8476f 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -2463,4 +2463,13 @@ 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.30.1


^ permalink raw reply related	[flat|nested] 54+ messages in thread

* [PATCH v1 2/7] PCI: Convert __pci_read_base() to __pci_bus_read_base()
  2021-03-08 12:20 [rfc, PATCH v1 0/7] PCI: introduce p2sb helper Andy Shevchenko
  2021-03-08 12:20 ` [PATCH v1 1/7] PCI: Introduce pci_bus_*() printing macros when device is not available Andy Shevchenko
@ 2021-03-08 12:20 ` Andy Shevchenko
  2021-03-10 17:14   ` Christoph Hellwig
  2021-03-08 12:20 ` [PATCH v1 3/7] PCI: New Primary to Sideband (P2SB) bridge support library Andy Shevchenko
                   ` (7 subsequent siblings)
  9 siblings, 1 reply; 54+ messages in thread
From: Andy Shevchenko @ 2021-03-08 12:20 UTC (permalink / raw)
  To: Wolfram Sang, Jean Delvare, Lee Jones, Andy Shevchenko,
	Tan Jui Nee, Jim Quinlan, Jonathan Yong, Bjorn Helgaas,
	linux-kernel, linux-i2c, linux-pci
  Cc: Jean Delvare, Peter Tyser, hdegoede, henning.schild

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   | 13 ++++++++-
 drivers/pci/probe.c | 69 +++++++++++++++++++++++----------------------
 2 files changed, 48 insertions(+), 34 deletions(-)

diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
index ef7c4661314f..58a0e9f7a530 100644
--- a/drivers/pci/pci.h
+++ b/drivers/pci/pci.h
@@ -258,8 +258,19 @@ 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_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);
+		    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 7d67be52d8e5..8cf139724a42 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -129,7 +129,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;
@@ -165,16 +165,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;
@@ -184,20 +189,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.
@@ -216,7 +219,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;
@@ -236,26 +239,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;
 	}
 
@@ -265,8 +267,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;
 		}
 
@@ -275,8 +278,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;
 		}
 	}
@@ -284,8 +288,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
@@ -302,18 +306,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.30.1


^ permalink raw reply related	[flat|nested] 54+ messages in thread

* [PATCH v1 3/7] PCI: New Primary to Sideband (P2SB) bridge support library
  2021-03-08 12:20 [rfc, PATCH v1 0/7] PCI: introduce p2sb helper Andy Shevchenko
  2021-03-08 12:20 ` [PATCH v1 1/7] PCI: Introduce pci_bus_*() printing macros when device is not available Andy Shevchenko
  2021-03-08 12:20 ` [PATCH v1 2/7] PCI: Convert __pci_read_base() to __pci_bus_read_base() Andy Shevchenko
@ 2021-03-08 12:20 ` Andy Shevchenko
  2021-03-08 18:52   ` Bjorn Helgaas
  2021-03-13  9:45   ` Henning Schild
  2021-03-08 12:20 ` [PATCH v1 4/7] mfd: lpc_ich: Factor out lpc_ich_enable_spi_write() Andy Shevchenko
                   ` (6 subsequent siblings)
  9 siblings, 2 replies; 54+ messages in thread
From: Andy Shevchenko @ 2021-03-08 12:20 UTC (permalink / raw)
  To: Wolfram Sang, Jean Delvare, Lee Jones, Andy Shevchenko,
	Tan Jui Nee, Jim Quinlan, Jonathan Yong, Bjorn Helgaas,
	linux-kernel, linux-i2c, linux-pci
  Cc: Jean Delvare, Peter Tyser, hdegoede, henning.schild

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

There is already one and at least one more user is coming which
requires an access to Primary to Sideband bridge (P2SB) in order to
get IO or MMIO bar hidden by BIOS. Create a library to access P2SB
for x86 devices.

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/pci/Kconfig      |  8 ++++
 drivers/pci/Makefile     |  1 +
 drivers/pci/pci-p2sb.c   | 83 ++++++++++++++++++++++++++++++++++++++++
 include/linux/pci-p2sb.h | 28 ++++++++++++++
 4 files changed, 120 insertions(+)
 create mode 100644 drivers/pci/pci-p2sb.c
 create mode 100644 include/linux/pci-p2sb.h

diff --git a/drivers/pci/Kconfig b/drivers/pci/Kconfig
index 0c473d75e625..740e5b30d6fd 100644
--- a/drivers/pci/Kconfig
+++ b/drivers/pci/Kconfig
@@ -252,6 +252,14 @@ config PCIE_BUS_PEER2PEER
 
 endchoice
 
+config PCI_P2SB
+	bool "Primary to Sideband (P2SB) bridge access support"
+	depends on PCI && X86
+	help
+	  The Primary to Sideband 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.
+
 source "drivers/pci/hotplug/Kconfig"
 source "drivers/pci/controller/Kconfig"
 source "drivers/pci/endpoint/Kconfig"
diff --git a/drivers/pci/Makefile b/drivers/pci/Makefile
index d62c4ac4ae1b..eee8d5dda7d9 100644
--- a/drivers/pci/Makefile
+++ b/drivers/pci/Makefile
@@ -23,6 +23,7 @@ obj-$(CONFIG_PCI_IOV)		+= iov.o
 obj-$(CONFIG_PCI_BRIDGE_EMUL)	+= pci-bridge-emul.o
 obj-$(CONFIG_PCI_LABEL)		+= pci-label.o
 obj-$(CONFIG_X86_INTEL_MID)	+= pci-mid.o
+obj-$(CONFIG_PCI_P2SB)		+= pci-p2sb.o
 obj-$(CONFIG_PCI_SYSCALL)	+= syscall.o
 obj-$(CONFIG_PCI_STUB)		+= pci-stub.o
 obj-$(CONFIG_PCI_PF_STUB)	+= pci-pf-stub.o
diff --git a/drivers/pci/pci-p2sb.c b/drivers/pci/pci-p2sb.c
new file mode 100644
index 000000000000..68d7dad48cdb
--- /dev/null
+++ b/drivers/pci/pci-p2sb.c
@@ -0,0 +1,83 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Primary to Sideband bridge (P2SB) 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-p2sb.h>
+
+#include <asm/cpu_device_id.h>
+#include <asm/intel-family.h>
+
+#include "pci.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 pci_p2sb_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;
+}
+
+/**
+ * pci_p2sb_bar - Get Primary to Sideband bridge (P2SB) device BAR
+ * @pdev:	PCI device to get a 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.
+ *
+ * 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 pci_p2sb_bar(struct pci_dev *pdev, unsigned int devfn, struct resource *mem)
+{
+	struct pci_bus *bus = pdev->bus;
+	unsigned int df;
+	int ret;
+
+	/* Get devfn for P2SB device itself */
+	ret = pci_p2sb_devfn(&df);
+	if (ret)
+		return ret;
+
+	pci_lock_rescan_remove();
+
+	/* Unhide the P2SB device */
+	pci_bus_write_config_byte(bus, df, 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, df, P2SBC_HIDE_BYTE, P2SBC_HIDE_BIT);
+
+	pci_unlock_rescan_remove();
+
+	pci_bus_info(bus, devfn, "BAR: %pR\n", mem);
+	return 0;
+}
+EXPORT_SYMBOL_GPL(pci_p2sb_bar);
diff --git a/include/linux/pci-p2sb.h b/include/linux/pci-p2sb.h
new file mode 100644
index 000000000000..15dd42737c84
--- /dev/null
+++ b/include/linux/pci-p2sb.h
@@ -0,0 +1,28 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Primary to Sideband bridge (P2SB) access support
+ */
+
+#ifndef _PCI_P2SB_H
+#define _PCI_P2SB_H
+
+#include <linux/errno.h>
+
+struct pci_dev;
+struct resource;
+
+#if IS_BUILTIN(CONFIG_PCI_P2SB)
+
+int pci_p2sb_bar(struct pci_dev *pdev, unsigned int devfn, struct resource *mem);
+
+#else /* CONFIG_PCI_P2SB is not set */
+
+static inline
+int pci_p2sb_bar(struct pci_dev *pdev, unsigned int devfn, struct resource *mem)
+{
+	return -ENODEV;
+}
+
+#endif /* CONFIG_PCI_P2SB */
+
+#endif /* _PCI_P2SB_H */
-- 
2.30.1


^ permalink raw reply related	[flat|nested] 54+ messages in thread

* [PATCH v1 4/7] mfd: lpc_ich: Factor out lpc_ich_enable_spi_write()
  2021-03-08 12:20 [rfc, PATCH v1 0/7] PCI: introduce p2sb helper Andy Shevchenko
                   ` (2 preceding siblings ...)
  2021-03-08 12:20 ` [PATCH v1 3/7] PCI: New Primary to Sideband (P2SB) bridge support library Andy Shevchenko
@ 2021-03-08 12:20 ` Andy Shevchenko
  2021-03-10 10:29   ` Lee Jones
  2021-03-08 12:20 ` [PATCH v1 5/7] mfd: lpc_ich: Switch to generic pci_p2sb_bar() Andy Shevchenko
                   ` (5 subsequent siblings)
  9 siblings, 1 reply; 54+ messages in thread
From: Andy Shevchenko @ 2021-03-08 12:20 UTC (permalink / raw)
  To: Wolfram Sang, Jean Delvare, Lee Jones, Andy Shevchenko,
	Tan Jui Nee, Jim Quinlan, Jonathan Yong, Bjorn Helgaas,
	linux-kernel, linux-i2c, linux-pci
  Cc: Jean Delvare, Peter Tyser, hdegoede, henning.schild

Factor out duplicate code to lpc_ich_enable_spi_write() helper function.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 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 3bbb29a7e7a5..3a19ed57260e 100644
--- a/drivers/mfd/lpc_ich.c
+++ b/drivers/mfd/lpc_ich.c
@@ -1083,12 +1083,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)
@@ -1112,8 +1121,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;
 
@@ -1134,8 +1142,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.30.1


^ permalink raw reply related	[flat|nested] 54+ messages in thread

* [PATCH v1 5/7] mfd: lpc_ich: Switch to generic pci_p2sb_bar()
  2021-03-08 12:20 [rfc, PATCH v1 0/7] PCI: introduce p2sb helper Andy Shevchenko
                   ` (3 preceding siblings ...)
  2021-03-08 12:20 ` [PATCH v1 4/7] mfd: lpc_ich: Factor out lpc_ich_enable_spi_write() Andy Shevchenko
@ 2021-03-08 12:20 ` Andy Shevchenko
  2021-03-10 10:35   ` Lee Jones
  2021-03-08 12:20 ` [PATCH v1 6/7] mfd: lpc_ich: Add support for pinctrl in non-ACPI system Andy Shevchenko
                   ` (4 subsequent siblings)
  9 siblings, 1 reply; 54+ messages in thread
From: Andy Shevchenko @ 2021-03-08 12:20 UTC (permalink / raw)
  To: Wolfram Sang, Jean Delvare, Lee Jones, Andy Shevchenko,
	Tan Jui Nee, Jim Quinlan, Jonathan Yong, Bjorn Helgaas,
	linux-kernel, linux-i2c, linux-pci
  Cc: Jean Delvare, Peter Tyser, hdegoede, henning.schild

Instead of open coding pci_p2sb_bar() functionality we are going to
use generic library for that. There one more user of it is coming.

Besides cleaning up it fixes a potential issue if, by some reason,
SPI bar is 64-bit.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 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 a03de3f7a8ed..c16bec1852e5 100644
--- a/drivers/mfd/Kconfig
+++ b/drivers/mfd/Kconfig
@@ -553,6 +553,7 @@ config LPC_ICH
 	tristate "Intel ICH LPC"
 	depends on PCI
 	select MFD_CORE
+	select PCI_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 3a19ed57260e..8e9bd6813287 100644
--- a/drivers/mfd/lpc_ich.c
+++ b/drivers/mfd/lpc_ich.c
@@ -42,6 +42,7 @@
 #include <linux/errno.h>
 #include <linux/acpi.h>
 #include <linux/pci.h>
+#include <linux/pci-p2sb.h>
 #include <linux/mfd/core.h>
 #include <linux/mfd/lpc_ich.h>
 #include <linux/platform_data/itco_wdt.h>
@@ -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
@@ -1126,26 +1125,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 = pci_p2sb_bar(dev, 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.30.1


^ permalink raw reply related	[flat|nested] 54+ messages in thread

* [PATCH v1 6/7] mfd: lpc_ich: Add support for pinctrl in non-ACPI system
  2021-03-08 12:20 [rfc, PATCH v1 0/7] PCI: introduce p2sb helper Andy Shevchenko
                   ` (4 preceding siblings ...)
  2021-03-08 12:20 ` [PATCH v1 5/7] mfd: lpc_ich: Switch to generic pci_p2sb_bar() Andy Shevchenko
@ 2021-03-08 12:20 ` Andy Shevchenko
  2021-03-10 10:27   ` Lee Jones
  2021-04-12 16:01   ` Henning Schild
  2021-03-08 12:20 ` [PATCH v1 7/7] i2c: i801: convert to use common P2SB accessor Andy Shevchenko
                   ` (3 subsequent siblings)
  9 siblings, 2 replies; 54+ messages in thread
From: Andy Shevchenko @ 2021-03-08 12:20 UTC (permalink / raw)
  To: Wolfram Sang, Jean Delvare, Lee Jones, Andy Shevchenko,
	Tan Jui Nee, Jim Quinlan, Jonathan Yong, Bjorn Helgaas,
	linux-kernel, linux-i2c, linux-pci
  Cc: Jean Delvare, Peter Tyser, hdegoede, henning.schild

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>
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/mfd/lpc_ich.c | 100 +++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 99 insertions(+), 1 deletion(-)

diff --git a/drivers/mfd/lpc_ich.c b/drivers/mfd/lpc_ich.c
index 8e9bd6813287..959247b6987a 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>
  *
@@ -43,6 +44,7 @@
 #include <linux/acpi.h>
 #include <linux/pci.h>
 #include <linux/pci-p2sb.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,73 @@ static struct mfd_cell lpc_ich_gpio_cell = {
 	.ignore_resource_conflicts = true,
 };
 
+/* Offset data for Apollo Lake GPIO controllers */
+#define APL_GPIO_SOUTHWEST_OFFSET	0xc00000
+#define APL_GPIO_SOUTHWEST_SIZE		0x654
+#define APL_GPIO_NORTHWEST_OFFSET	0xc40000
+#define APL_GPIO_NORTHWEST_SIZE		0x764
+#define APL_GPIO_NORTH_OFFSET		0xc50000
+#define APL_GPIO_NORTH_SIZE		0x76c
+#define APL_GPIO_WEST_OFFSET		0xc70000
+#define APL_GPIO_WEST_SIZE		0x674
+
+#define APL_GPIO_NR_DEVICES		4
+#define APL_GPIO_IRQ			14
+
+static struct resource apl_gpio_resources[APL_GPIO_NR_DEVICES][2] = {
+	{
+		DEFINE_RES_MEM(APL_GPIO_NORTH_OFFSET, APL_GPIO_NORTH_SIZE),
+		DEFINE_RES_IRQ(APL_GPIO_IRQ),
+	},
+	{
+		DEFINE_RES_MEM(APL_GPIO_NORTHWEST_OFFSET, APL_GPIO_NORTHWEST_SIZE),
+		DEFINE_RES_IRQ(APL_GPIO_IRQ),
+	},
+	{
+		DEFINE_RES_MEM(APL_GPIO_WEST_OFFSET, APL_GPIO_WEST_SIZE),
+		DEFINE_RES_IRQ(APL_GPIO_IRQ),
+	},
+	{
+		DEFINE_RES_MEM(APL_GPIO_SOUTHWEST_OFFSET, APL_GPIO_SOUTHWEST_SIZE),
+		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] = {
+	{
+		/* North */
+		.name = "apollolake-pinctrl",
+		.id = 0,
+		.num_resources = ARRAY_SIZE(apl_gpio_resources[0]),
+		.resources = apl_gpio_resources[0],
+		.ignore_resource_conflicts = true,
+	},
+	{
+		/* NorthWest */
+		.name = "apollolake-pinctrl",
+		.id = 1,
+		.num_resources = ARRAY_SIZE(apl_gpio_resources[1]),
+		.resources = apl_gpio_resources[1],
+		.ignore_resource_conflicts = true,
+	},
+	{
+		/* West */
+		.name = "apollolake-pinctrl",
+		.id = 2,
+		.num_resources = ARRAY_SIZE(apl_gpio_resources[2]),
+		.resources = apl_gpio_resources[2],
+		.ignore_resource_conflicts = true,
+	},
+	{
+		/* SouthWest */
+		.name = "apollolake-pinctrl",
+		.id = 3,
+		.num_resources = ARRAY_SIZE(apl_gpio_resources[3]),
+		.resources = apl_gpio_resources[3],
+		.ignore_resource_conflicts = true,
+	},
+};
 
 static struct mfd_cell lpc_ich_spi_cell = {
 	.name = "intel-spi",
@@ -1082,6 +1151,29 @@ 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;
+
+	ret = pci_p2sb_bar(dev, PCI_DEVFN(13, 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)
 {
@@ -1198,6 +1290,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.30.1


^ permalink raw reply related	[flat|nested] 54+ messages in thread

* [PATCH v1 7/7] i2c: i801: convert to use common P2SB accessor
  2021-03-08 12:20 [rfc, PATCH v1 0/7] PCI: introduce p2sb helper Andy Shevchenko
                   ` (5 preceding siblings ...)
  2021-03-08 12:20 ` [PATCH v1 6/7] mfd: lpc_ich: Add support for pinctrl in non-ACPI system Andy Shevchenko
@ 2021-03-08 12:20 ` Andy Shevchenko
  2021-03-10 14:51   ` Jean Delvare
  2021-03-13  9:25 ` [rfc, PATCH v1 0/7] PCI: introduce p2sb helper Henning Schild
                   ` (2 subsequent siblings)
  9 siblings, 1 reply; 54+ messages in thread
From: Andy Shevchenko @ 2021-03-08 12:20 UTC (permalink / raw)
  To: Wolfram Sang, Jean Delvare, Lee Jones, Andy Shevchenko,
	Tan Jui Nee, Jim Quinlan, Jonathan Yong, Bjorn Helgaas,
	linux-kernel, linux-i2c, linux-pci
  Cc: Jean Delvare, Peter Tyser, hdegoede, henning.schild

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 | 40 ++++++++---------------------------
 drivers/pci/pci-p2sb.c        |  6 ++++++
 3 files changed, 16 insertions(+), 31 deletions(-)

diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
index 05ebf7546e3f..ffd3007f888c 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 PCI_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 4acee6f9e5a3..23b43de9786a 100644
--- a/drivers/i2c/busses/i2c-i801.c
+++ b/drivers/i2c/busses/i2c-i801.c
@@ -90,6 +90,7 @@
 #include <linux/interrupt.h>
 #include <linux/module.h>
 #include <linux/pci.h>
+#include <linux/pci-p2sb.h>
 #include <linux/kernel.h>
 #include <linux/stddef.h>
 #include <linux/delay.h>
@@ -136,7 +137,6 @@
 #define TCOBASE		0x050
 #define TCOCTL		0x054
 
-#define SBREG_BAR		0x10
 #define SBREG_SMBCTRL		0xc6000c
 #define SBREG_SMBCTRL_DNV	0xcf000c
 
@@ -1524,52 +1524,30 @@ static const struct itco_wdt_platform_data spt_tco_platform_data = {
 	.version = 4,
 };
 
-static DEFINE_SPINLOCK(p2sb_spinlock);
-
 static struct platform_device *
 i801_add_tco_spt(struct i801_priv *priv, struct pci_dev *pci_dev,
 		 struct resource *tco_res)
 {
 	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.
+	 * bridge (P2SB).
 	 */
-	spin_lock(&p2sb_spinlock);
 
 	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);
-	spin_unlock(&p2sb_spinlock);
-
 	res = &tco_res[1];
+	ret = pci_p2sb_bar(pci_dev, devfn, 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->end = res->start + 3;
-	res->flags = IORESOURCE_MEM;
+		res->start += SBREG_SMBCTRL;
 
 	return platform_device_register_resndata(&pci_dev->dev, "iTCO_wdt", -1,
 					tco_res, 2, &spt_tco_platform_data,
diff --git a/drivers/pci/pci-p2sb.c b/drivers/pci/pci-p2sb.c
index 68d7dad48cdb..7f6bc7d4482a 100644
--- a/drivers/pci/pci-p2sb.c
+++ b/drivers/pci/pci-p2sb.c
@@ -22,6 +22,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.30.1


^ permalink raw reply related	[flat|nested] 54+ messages in thread

* Re: [PATCH v1 3/7] PCI: New Primary to Sideband (P2SB) bridge support library
  2021-03-08 12:20 ` [PATCH v1 3/7] PCI: New Primary to Sideband (P2SB) bridge support library Andy Shevchenko
@ 2021-03-08 18:52   ` Bjorn Helgaas
  2021-03-08 19:16     ` Andy Shevchenko
  2021-03-13  9:45   ` Henning Schild
  1 sibling, 1 reply; 54+ messages in thread
From: Bjorn Helgaas @ 2021-03-08 18:52 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: Wolfram Sang, Jean Delvare, Lee Jones, Tan Jui Nee, Jim Quinlan,
	Jonathan Yong, Bjorn Helgaas, linux-kernel, linux-i2c, linux-pci,
	Jean Delvare, Peter Tyser, hdegoede, henning.schild

On Mon, Mar 08, 2021 at 02:20:16PM +0200, Andy Shevchenko wrote:
> From: Jonathan Yong <jonathan.yong@intel.com>
> 
> There is already one and at least one more user is coming which
> requires an access to Primary to Sideband bridge (P2SB) in order to
> get IO or MMIO bar hidden by BIOS. Create a library to access P2SB
> for x86 devices.

Can you include a spec reference?  I'm trying to figure out why this
belongs in drivers/pci/.  It looks very device-specific.

> 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/pci/Kconfig      |  8 ++++
>  drivers/pci/Makefile     |  1 +
>  drivers/pci/pci-p2sb.c   | 83 ++++++++++++++++++++++++++++++++++++++++
>  include/linux/pci-p2sb.h | 28 ++++++++++++++
>  4 files changed, 120 insertions(+)
>  create mode 100644 drivers/pci/pci-p2sb.c
>  create mode 100644 include/linux/pci-p2sb.h
> 
> diff --git a/drivers/pci/Kconfig b/drivers/pci/Kconfig
> index 0c473d75e625..740e5b30d6fd 100644
> --- a/drivers/pci/Kconfig
> +++ b/drivers/pci/Kconfig
> @@ -252,6 +252,14 @@ config PCIE_BUS_PEER2PEER
>  
>  endchoice
>  
> +config PCI_P2SB
> +	bool "Primary to Sideband (P2SB) bridge access support"
> +	depends on PCI && X86
> +	help
> +	  The Primary to Sideband 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.

This doesn't sound like a "bridge".  If it's a bridge, what's on the
primary (upstream) side?  What's on the secondary side?  What
resources are passed through the bridge, i.e., what transactions does
it transfer from one side to the other?

>  source "drivers/pci/hotplug/Kconfig"
>  source "drivers/pci/controller/Kconfig"
>  source "drivers/pci/endpoint/Kconfig"
> diff --git a/drivers/pci/Makefile b/drivers/pci/Makefile
> index d62c4ac4ae1b..eee8d5dda7d9 100644
> --- a/drivers/pci/Makefile
> +++ b/drivers/pci/Makefile
> @@ -23,6 +23,7 @@ obj-$(CONFIG_PCI_IOV)		+= iov.o
>  obj-$(CONFIG_PCI_BRIDGE_EMUL)	+= pci-bridge-emul.o
>  obj-$(CONFIG_PCI_LABEL)		+= pci-label.o
>  obj-$(CONFIG_X86_INTEL_MID)	+= pci-mid.o
> +obj-$(CONFIG_PCI_P2SB)		+= pci-p2sb.o
>  obj-$(CONFIG_PCI_SYSCALL)	+= syscall.o
>  obj-$(CONFIG_PCI_STUB)		+= pci-stub.o
>  obj-$(CONFIG_PCI_PF_STUB)	+= pci-pf-stub.o
> diff --git a/drivers/pci/pci-p2sb.c b/drivers/pci/pci-p2sb.c
> new file mode 100644
> index 000000000000..68d7dad48cdb
> --- /dev/null
> +++ b/drivers/pci/pci-p2sb.c
> @@ -0,0 +1,83 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Primary to Sideband bridge (P2SB) 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-p2sb.h>
> +
> +#include <asm/cpu_device_id.h>
> +#include <asm/intel-family.h>
> +
> +#include "pci.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 pci_p2sb_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;
> +}
> +
> +/**
> + * pci_p2sb_bar - Get Primary to Sideband bridge (P2SB) device BAR
> + * @pdev:	PCI device to get a 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.
> + *
> + * 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 pci_p2sb_bar(struct pci_dev *pdev, unsigned int devfn, struct resource *mem)
> +{
> +	struct pci_bus *bus = pdev->bus;
> +	unsigned int df;
> +	int ret;
> +
> +	/* Get devfn for P2SB device itself */
> +	ret = pci_p2sb_devfn(&df);
> +	if (ret)
> +		return ret;
> +
> +	pci_lock_rescan_remove();
> +
> +	/* Unhide the P2SB device */
> +	pci_bus_write_config_byte(bus, df, 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);

I don't get this.  Apparently this normally hidden device is consuming
PCI address space.  The PCI core needs to know about this.  If it
doesn't, the PCI core may assign this space to another device.

> +	/* Hide the P2SB device */
> +	pci_bus_write_config_byte(bus, df, P2SBC_HIDE_BYTE, P2SBC_HIDE_BIT);
> +
> +	pci_unlock_rescan_remove();
> +
> +	pci_bus_info(bus, devfn, "BAR: %pR\n", mem);
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(pci_p2sb_bar);
> diff --git a/include/linux/pci-p2sb.h b/include/linux/pci-p2sb.h
> new file mode 100644
> index 000000000000..15dd42737c84
> --- /dev/null
> +++ b/include/linux/pci-p2sb.h
> @@ -0,0 +1,28 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * Primary to Sideband bridge (P2SB) access support
> + */
> +
> +#ifndef _PCI_P2SB_H
> +#define _PCI_P2SB_H
> +
> +#include <linux/errno.h>
> +
> +struct pci_dev;
> +struct resource;
> +
> +#if IS_BUILTIN(CONFIG_PCI_P2SB)
> +
> +int pci_p2sb_bar(struct pci_dev *pdev, unsigned int devfn, struct resource *mem);
> +
> +#else /* CONFIG_PCI_P2SB is not set */
> +
> +static inline
> +int pci_p2sb_bar(struct pci_dev *pdev, unsigned int devfn, struct resource *mem)
> +{
> +	return -ENODEV;
> +}
> +
> +#endif /* CONFIG_PCI_P2SB */
> +
> +#endif /* _PCI_P2SB_H */
> -- 
> 2.30.1
> 

^ permalink raw reply	[flat|nested] 54+ messages in thread

* Re: [PATCH v1 3/7] PCI: New Primary to Sideband (P2SB) bridge support library
  2021-03-08 18:52   ` Bjorn Helgaas
@ 2021-03-08 19:16     ` Andy Shevchenko
  2021-03-09  1:42       ` Bjorn Helgaas
  0 siblings, 1 reply; 54+ messages in thread
From: Andy Shevchenko @ 2021-03-08 19:16 UTC (permalink / raw)
  To: Bjorn Helgaas
  Cc: Wolfram Sang, Jean Delvare, Lee Jones, Tan Jui Nee, Jim Quinlan,
	Jonathan Yong, Bjorn Helgaas, linux-kernel, linux-i2c, linux-pci,
	Jean Delvare, Peter Tyser, hdegoede, henning.schild

On Mon, Mar 08, 2021 at 12:52:12PM -0600, Bjorn Helgaas wrote:
> On Mon, Mar 08, 2021 at 02:20:16PM +0200, Andy Shevchenko wrote:
> > From: Jonathan Yong <jonathan.yong@intel.com>
> > 
> > There is already one and at least one more user is coming which
> > requires an access to Primary to Sideband bridge (P2SB) in order to
> > get IO or MMIO bar hidden by BIOS. Create a library to access P2SB
> > for x86 devices.
> 
> Can you include a spec reference?

I'm not sure I have a public link to the spec. It's the 100 Series PCH [1].
The document number to look for is 546955 [2] and there actually a bit of
information about this.

> I'm trying to figure out why this
> belongs in drivers/pci/.  It looks very device-specific.

Because it's all about access to PCI configuration spaces of the (hidden)
devices.

[1]: https://ark.intel.com/content/www/us/en/ark/products/series/98456/intel-100-series-desktop-chipsets.html
[2]: https://medium.com/@jacksonchen_43335/bios-gpio-p2sb-70e9b829b403

...

> > +config PCI_P2SB
> > +	bool "Primary to Sideband (P2SB) bridge access support"
> > +	depends on PCI && X86
> > +	help
> > +	  The Primary to Sideband 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.
> 
> This doesn't sound like a "bridge".  If it's a bridge, what's on the
> primary (upstream) side?  What's on the secondary side?  What
> resources are passed through the bridge, i.e., what transactions does
> it transfer from one side to the other?

It's a confusion terminology here. It's a Bridge according to the spec, but
it is *not* a PCI Bridge as you may had a first impression.

...

> > +	/* Unhide the P2SB device */
> > +	pci_bus_write_config_byte(bus, df, 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);
> 
> I don't get this.  Apparently this normally hidden device is consuming
> PCI address space.  The PCI core needs to know about this.  If it
> doesn't, the PCI core may assign this space to another device.

Right, it returns all 1:s to any request so PCI core *thinks* it's plugged off
(like D3cold or so).

> > +	/* Hide the P2SB device */
> > +	pci_bus_write_config_byte(bus, df, P2SBC_HIDE_BYTE, P2SBC_HIDE_BIT);

-- 
With Best Regards,
Andy Shevchenko



^ permalink raw reply	[flat|nested] 54+ messages in thread

* Re: [PATCH v1 3/7] PCI: New Primary to Sideband (P2SB) bridge support library
  2021-03-08 19:16     ` Andy Shevchenko
@ 2021-03-09  1:42       ` Bjorn Helgaas
  2021-03-09  8:42         ` Henning Schild
  2021-11-26 15:38         ` Andy Shevchenko
  0 siblings, 2 replies; 54+ messages in thread
From: Bjorn Helgaas @ 2021-03-09  1:42 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: Wolfram Sang, Jean Delvare, Lee Jones, Tan Jui Nee, Jim Quinlan,
	Jonathan Yong, Bjorn Helgaas, linux-kernel, linux-i2c, linux-pci,
	Jean Delvare, Peter Tyser, hdegoede, henning.schild

On Mon, Mar 08, 2021 at 09:16:50PM +0200, Andy Shevchenko wrote:
> On Mon, Mar 08, 2021 at 12:52:12PM -0600, Bjorn Helgaas wrote:
> > On Mon, Mar 08, 2021 at 02:20:16PM +0200, Andy Shevchenko wrote:
> > > From: Jonathan Yong <jonathan.yong@intel.com>
> > > 
> > > There is already one and at least one more user is coming which
> > > requires an access to Primary to Sideband bridge (P2SB) in order to
> > > get IO or MMIO bar hidden by BIOS. Create a library to access P2SB
> > > for x86 devices.
> > 
> > Can you include a spec reference?
> 
> I'm not sure I have a public link to the spec. It's the 100 Series PCH [1].
> The document number to look for is 546955 [2] and there actually a bit of
> information about this.

This link, found by googling for "p2sb bridge", looks like it might
have relevant public links:

https://lab.whitequark.org/notes/2017-11-08/accessing-intel-ich-pch-gpios/

I'd prefer if you could dig out the relevant sections because I really
don't know how to identify them.

> > I'm trying to figure out why this
> > belongs in drivers/pci/.  It looks very device-specific.
> 
> Because it's all about access to PCI configuration spaces of the (hidden)
> devices.

The PCI core generally doesn't deal with device-specific config
registers.

> [1]: https://ark.intel.com/content/www/us/en/ark/products/series/98456/intel-100-series-desktop-chipsets.html
> [2]: https://medium.com/@jacksonchen_43335/bios-gpio-p2sb-70e9b829b403
> 
> ...
> 
> > > +config PCI_P2SB
> > > +	bool "Primary to Sideband (P2SB) bridge access support"
> > > +	depends on PCI && X86
> > > +	help
> > > +	  The Primary to Sideband 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.
> > 
> > This doesn't sound like a "bridge".  If it's a bridge, what's on the
> > primary (upstream) side?  What's on the secondary side?  What
> > resources are passed through the bridge, i.e., what transactions does
> > it transfer from one side to the other?
> 
> It's a confusion terminology here. It's a Bridge according to the spec, but
> it is *not* a PCI Bridge as you may had a first impression.

The code suggests that a register on this device controls whether a
different device is visible in config space.  I think it will be
better if we can describe what's happening.

> ...
> 
> > > +	/* Unhide the P2SB device */
> > > +	pci_bus_write_config_byte(bus, df, 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);
> > 
> > I don't get this.  Apparently this normally hidden device is consuming
> > PCI address space.  The PCI core needs to know about this.  If it
> > doesn't, the PCI core may assign this space to another device.
> 
> Right, it returns all 1:s to any request so PCI core *thinks* it's
> plugged off (like D3cold or so).

I'm asking about the MMIO address space.  The BAR is a register in
config space.  AFAICT, clearing P2SBC_HIDE_BYTE makes that BAR
visible.  The BAR describes a region of PCI address space.  It looks
like setting P2SBC_HIDE_BIT makes the BAR disappear from config space,
but it sounds like the PCI address space *described* by the BAR is
still claimed by the device.  If the device didn't respond to that
MMIO space, you would have no reason to read the BAR at all.

So what keeps the PCI core from assigning that MMIO space to another
device?

This all sounds quite irregular from the point of view of the PCI
core.  If a device responds to address space that is not described by
a standard PCI BAR, or by an EA capability, or by one of the legacy
VGA or IDE exceptions, we have a problem.  That space must be
described *somehow* in a generic way, e.g., ACPI or similar.

What happens if CONFIG_PCI_P2SB is unset?  The device doesn't know
that, and if it is still consuming MMIO address space that we don't
know about, that's a problem.

> > > +	/* Hide the P2SB device */
> > > +	pci_bus_write_config_byte(bus, df, P2SBC_HIDE_BYTE, P2SBC_HIDE_BIT);
> 
> -- 
> With Best Regards,
> Andy Shevchenko
> 
> 

^ permalink raw reply	[flat|nested] 54+ messages in thread

* Re: [PATCH v1 3/7] PCI: New Primary to Sideband (P2SB) bridge support library
  2021-03-09  1:42       ` Bjorn Helgaas
@ 2021-03-09  8:42         ` Henning Schild
  2021-04-01 15:45           ` Andy Shevchenko
  2021-04-02 13:09           ` Enrico Weigelt, metux IT consult
  2021-11-26 15:38         ` Andy Shevchenko
  1 sibling, 2 replies; 54+ messages in thread
From: Henning Schild @ 2021-03-09  8:42 UTC (permalink / raw)
  To: Bjorn Helgaas
  Cc: Andy Shevchenko, Wolfram Sang, Jean Delvare, Lee Jones,
	Tan Jui Nee, Jim Quinlan, Jonathan Yong, Bjorn Helgaas,
	linux-kernel, linux-i2c, linux-pci, Jean Delvare, Peter Tyser,
	hdegoede

Am Mon, 8 Mar 2021 19:42:21 -0600
schrieb Bjorn Helgaas <helgaas@kernel.org>:

> On Mon, Mar 08, 2021 at 09:16:50PM +0200, Andy Shevchenko wrote:
> > On Mon, Mar 08, 2021 at 12:52:12PM -0600, Bjorn Helgaas wrote:  
> > > On Mon, Mar 08, 2021 at 02:20:16PM +0200, Andy Shevchenko wrote:  
> > > > From: Jonathan Yong <jonathan.yong@intel.com>
> > > > 
> > > > There is already one and at least one more user is coming which
> > > > requires an access to Primary to Sideband bridge (P2SB) in
> > > > order to get IO or MMIO bar hidden by BIOS. Create a library to
> > > > access P2SB for x86 devices.  
> > > 
> > > Can you include a spec reference?  
> > 
> > I'm not sure I have a public link to the spec. It's the 100 Series
> > PCH [1]. The document number to look for is 546955 [2] and there
> > actually a bit of information about this.  
> 
> This link, found by googling for "p2sb bridge", looks like it might
> have relevant public links:
> 
> https://lab.whitequark.org/notes/2017-11-08/accessing-intel-ich-pch-gpios/
> 
> I'd prefer if you could dig out the relevant sections because I really
> don't know how to identify them.
> 
> > > I'm trying to figure out why this
> > > belongs in drivers/pci/.  It looks very device-specific.  
> > 
> > Because it's all about access to PCI configuration spaces of the
> > (hidden) devices.  
> 
> The PCI core generally doesn't deal with device-specific config
> registers.
> 
> > [1]:
> > https://ark.intel.com/content/www/us/en/ark/products/series/98456/intel-100-series-desktop-chipsets.html
> > [2]:
> > https://medium.com/@jacksonchen_43335/bios-gpio-p2sb-70e9b829b403
> > 
> > ...
> >   
> > > > +config PCI_P2SB
> > > > +	bool "Primary to Sideband (P2SB) bridge access support"
> > > > +	depends on PCI && X86
> > > > +	help
> > > > +	  The Primary to Sideband 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.  
> > > 
> > > This doesn't sound like a "bridge".  If it's a bridge, what's on
> > > the primary (upstream) side?  What's on the secondary side?  What
> > > resources are passed through the bridge, i.e., what transactions
> > > does it transfer from one side to the other?  
> > 
> > It's a confusion terminology here. It's a Bridge according to the
> > spec, but it is *not* a PCI Bridge as you may had a first
> > impression.  
> 
> The code suggests that a register on this device controls whether a
> different device is visible in config space.  I think it will be
> better if we can describe what's happening.
> 
> > ...
> >   
> > > > +	/* Unhide the P2SB device */
> > > > +	pci_bus_write_config_byte(bus, df, 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);  
> > > 
> > > I don't get this.  Apparently this normally hidden device is
> > > consuming PCI address space.  The PCI core needs to know about
> > > this.  If it doesn't, the PCI core may assign this space to
> > > another device.  
> > 
> > Right, it returns all 1:s to any request so PCI core *thinks* it's
> > plugged off (like D3cold or so).  
> 
> I'm asking about the MMIO address space.  The BAR is a register in
> config space.  AFAICT, clearing P2SBC_HIDE_BYTE makes that BAR
> visible.  The BAR describes a region of PCI address space.  It looks
> like setting P2SBC_HIDE_BIT makes the BAR disappear from config space,
> but it sounds like the PCI address space *described* by the BAR is
> still claimed by the device.  If the device didn't respond to that
> MMIO space, you would have no reason to read the BAR at all.
> 
> So what keeps the PCI core from assigning that MMIO space to another
> device?

The device will respond to MMIO while being hidden. I am afraid nothing
stops a collision, except for the assumption that the BIOS is always
right and PCI devices never get remapped. But just guessing here.

I have seen devices with coreboot having the P2SB visible, and most
likely relocatable. Making it visible in Linux and not hiding it again
might work, but probably only as long as Linux will not relocate it.
Which i am afraid might seriously upset the BIOS, depending on what a
device does with those GPIOs and which parts are implemented in the
BIOS.

regards,
Henning

> This all sounds quite irregular from the point of view of the PCI
> core.  If a device responds to address space that is not described by
> a standard PCI BAR, or by an EA capability, or by one of the legacy
> VGA or IDE exceptions, we have a problem.  That space must be
> described *somehow* in a generic way, e.g., ACPI or similar.
> 
> What happens if CONFIG_PCI_P2SB is unset?  The device doesn't know
> that, and if it is still consuming MMIO address space that we don't
> know about, that's a problem.
> 
> > > > +	/* Hide the P2SB device */
> > > > +	pci_bus_write_config_byte(bus, df, P2SBC_HIDE_BYTE,
> > > > P2SBC_HIDE_BIT);  
> > 
> > -- 
> > With Best Regards,
> > Andy Shevchenko
> > 
> >   


^ permalink raw reply	[flat|nested] 54+ messages in thread

* Re: [PATCH v1 6/7] mfd: lpc_ich: Add support for pinctrl in non-ACPI system
  2021-03-08 12:20 ` [PATCH v1 6/7] mfd: lpc_ich: Add support for pinctrl in non-ACPI system Andy Shevchenko
@ 2021-03-10 10:27   ` Lee Jones
  2021-04-12 16:01   ` Henning Schild
  1 sibling, 0 replies; 54+ messages in thread
From: Lee Jones @ 2021-03-10 10:27 UTC (permalink / raw)
  To: Andy Shevchenko, gregkh
  Cc: Wolfram Sang, Jean Delvare, Tan Jui Nee, Jim Quinlan,
	Jonathan Yong, Bjorn Helgaas, linux-kernel, linux-i2c, linux-pci,
	Jean Delvare, Peter Tyser, hdegoede, henning.schild

On Mon, 08 Mar 2021, Andy Shevchenko wrote:

> From: Tan Jui Nee <jui.nee.tan@intel.com>
> 
> Add support for non-ACPI systems, such as system that uses
> Advanced Boot Loader (ABL) whereby a platform device has to be created
> in order to bind with pin control and GPIO.
> 
> At the moment, Intel Apollo Lake In-Vehicle Infotainment (IVI) system
> requires a driver to hide and unhide P2SB to lookup P2SB BAR and pass
> the PCI BAR address to GPIO.
> 
> Signed-off-by: Tan Jui Nee <jui.nee.tan@intel.com>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  drivers/mfd/lpc_ich.c | 100 +++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 99 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/mfd/lpc_ich.c b/drivers/mfd/lpc_ich.c
> index 8e9bd6813287..959247b6987a 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

Big C or little c?  Please be consistent.

>   *  Copyright (c) 2011 Extreme Engineering Solution, Inc.
>   *  Author: Aaron Sierra <asierra@xes-inc.com>
>   *
> @@ -43,6 +44,7 @@
>  #include <linux/acpi.h>
>  #include <linux/pci.h>
>  #include <linux/pci-p2sb.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,73 @@ static struct mfd_cell lpc_ich_gpio_cell = {
>  	.ignore_resource_conflicts = true,
>  };
>  
> +/* Offset data for Apollo Lake GPIO controllers */
> +#define APL_GPIO_SOUTHWEST_OFFSET	0xc00000
> +#define APL_GPIO_SOUTHWEST_SIZE		0x654
> +#define APL_GPIO_NORTHWEST_OFFSET	0xc40000
> +#define APL_GPIO_NORTHWEST_SIZE		0x764
> +#define APL_GPIO_NORTH_OFFSET		0xc50000
> +#define APL_GPIO_NORTH_SIZE		0x76c
> +#define APL_GPIO_WEST_OFFSET		0xc70000
> +#define APL_GPIO_WEST_SIZE		0x674
> +
> +#define APL_GPIO_NR_DEVICES		4
> +#define APL_GPIO_IRQ			14
> +
> +static struct resource apl_gpio_resources[APL_GPIO_NR_DEVICES][2] = {
> +	{
> +		DEFINE_RES_MEM(APL_GPIO_NORTH_OFFSET, APL_GPIO_NORTH_SIZE),
> +		DEFINE_RES_IRQ(APL_GPIO_IRQ),
> +	},
> +	{
> +		DEFINE_RES_MEM(APL_GPIO_NORTHWEST_OFFSET, APL_GPIO_NORTHWEST_SIZE),
> +		DEFINE_RES_IRQ(APL_GPIO_IRQ),
> +	},
> +	{
> +		DEFINE_RES_MEM(APL_GPIO_WEST_OFFSET, APL_GPIO_WEST_SIZE),
> +		DEFINE_RES_IRQ(APL_GPIO_IRQ),
> +	},
> +	{
> +		DEFINE_RES_MEM(APL_GPIO_SOUTHWEST_OFFSET, APL_GPIO_SOUTHWEST_SIZE),
> +		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] = {
> +	{
> +		/* North */
> +		.name = "apollolake-pinctrl",
> +		.id = 0,

Do these have to be hard-coded?

> +		.num_resources = ARRAY_SIZE(apl_gpio_resources[0]),
> +		.resources = apl_gpio_resources[0],

You can make this less fragile by defining the index and using:

  [DEFINE_X_Y_Z] = { /* resource */ }, /* etc */

... above.

> +		.ignore_resource_conflicts = true,
> +	},
> +	{
> +		/* NorthWest */
> +		.name = "apollolake-pinctrl",
> +		.id = 1,
> +		.num_resources = ARRAY_SIZE(apl_gpio_resources[1]),
> +		.resources = apl_gpio_resources[1],
> +		.ignore_resource_conflicts = true,
> +	},
> +	{
> +		/* West */
> +		.name = "apollolake-pinctrl",
> +		.id = 2,
> +		.num_resources = ARRAY_SIZE(apl_gpio_resources[2]),
> +		.resources = apl_gpio_resources[2],
> +		.ignore_resource_conflicts = true,
> +	},
> +	{
> +		/* SouthWest */
> +		.name = "apollolake-pinctrl",
> +		.id = 3,
> +		.num_resources = ARRAY_SIZE(apl_gpio_resources[3]),
> +		.resources = apl_gpio_resources[3],
> +		.ignore_resource_conflicts = true,
> +	},
> +};
>  
>  static struct mfd_cell lpc_ich_spi_cell = {
>  	.name = "intel-spi",
> @@ -1082,6 +1151,29 @@ 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;
> +
> +	ret = pci_p2sb_bar(dev, PCI_DEVFN(13, 0), &base);

What is 13 and 0?  Should these be defined?

> +	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;
> +	}

So you're converting PCI devices to platform devices.

I'm not sure how 'okay' that is.

Adding Greg to see if he has an opinion.

> +	return mfd_add_devices(&dev->dev, 0, apl_gpio_devices,

Please use the defines, rather than 0.

> +			       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)
>  {
> @@ -1198,6 +1290,12 @@ static int lpc_ich_probe(struct pci_dev *dev,
>  			cell_added = true;
>  	}
>  
> +	if (priv->chipset == LPC_APL) {
> +		ret = lpc_ich_init_pinctrl(dev);
> +		if (!ret)
> +			cell_added = true;
> +	}
> +
>  	if (lpc_chipset_info[priv->chipset].spi_type) {
>  		ret = lpc_ich_init_spi(dev);
>  		if (!ret)

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

^ permalink raw reply	[flat|nested] 54+ messages in thread

* Re: [PATCH v1 4/7] mfd: lpc_ich: Factor out lpc_ich_enable_spi_write()
  2021-03-08 12:20 ` [PATCH v1 4/7] mfd: lpc_ich: Factor out lpc_ich_enable_spi_write() Andy Shevchenko
@ 2021-03-10 10:29   ` Lee Jones
  0 siblings, 0 replies; 54+ messages in thread
From: Lee Jones @ 2021-03-10 10:29 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: Wolfram Sang, Jean Delvare, Tan Jui Nee, Jim Quinlan,
	Jonathan Yong, Bjorn Helgaas, linux-kernel, linux-i2c, linux-pci,
	Jean Delvare, Peter Tyser, hdegoede, henning.schild

On Mon, 08 Mar 2021, Andy Shevchenko wrote:

> Factor out duplicate code to lpc_ich_enable_spi_write() helper function.
> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  drivers/mfd/lpc_ich.c | 17 ++++++++++++-----
>  1 file changed, 12 insertions(+), 5 deletions(-)

You are adding way more lines than you're saving here.

It's not a horrible change, so:

For my own reference (apply this as-is to your sign-off block):

  Acked-for-MFD-by: Lee Jones <lee.jones@linaro.org>

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

^ permalink raw reply	[flat|nested] 54+ messages in thread

* Re: [PATCH v1 5/7] mfd: lpc_ich: Switch to generic pci_p2sb_bar()
  2021-03-08 12:20 ` [PATCH v1 5/7] mfd: lpc_ich: Switch to generic pci_p2sb_bar() Andy Shevchenko
@ 2021-03-10 10:35   ` Lee Jones
  2021-03-10 12:05     ` Andy Shevchenko
  0 siblings, 1 reply; 54+ messages in thread
From: Lee Jones @ 2021-03-10 10:35 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: Wolfram Sang, Jean Delvare, Tan Jui Nee, Jim Quinlan,
	Jonathan Yong, Bjorn Helgaas, linux-kernel, linux-i2c, linux-pci,
	Jean Delvare, Peter Tyser, hdegoede, henning.schild

On Mon, 08 Mar 2021, Andy Shevchenko wrote:

> Instead of open coding pci_p2sb_bar() functionality we are going to
> use generic library for that. There one more user of it is coming.
> 
> Besides cleaning up it fixes a potential issue if, by some reason,
> SPI bar is 64-bit.

Probably worth cleaning up the English in both these sections.

 Instead of open coding pci_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.

Also worth briefly describing what that issue is I think.

> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  drivers/mfd/Kconfig   |  1 +
>  drivers/mfd/lpc_ich.c | 20 ++++++--------------
>  2 files changed, 7 insertions(+), 14 deletions(-)

Code looks fine:

For my own reference (apply this as-is to your sign-off block):

  Acked-for-MFD-by: Lee Jones <lee.jones@linaro.org>

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

^ permalink raw reply	[flat|nested] 54+ messages in thread

* Re: [PATCH v1 5/7] mfd: lpc_ich: Switch to generic pci_p2sb_bar()
  2021-03-10 10:35   ` Lee Jones
@ 2021-03-10 12:05     ` Andy Shevchenko
  2021-03-10 12:57       ` Lee Jones
  0 siblings, 1 reply; 54+ messages in thread
From: Andy Shevchenko @ 2021-03-10 12:05 UTC (permalink / raw)
  To: Lee Jones
  Cc: Wolfram Sang, Jean Delvare, Tan Jui Nee, Jim Quinlan,
	Jonathan Yong, Bjorn Helgaas, linux-kernel, linux-i2c, linux-pci,
	Jean Delvare, Peter Tyser, hdegoede, henning.schild

On Wed, Mar 10, 2021 at 10:35:39AM +0000, Lee Jones wrote:
> On Mon, 08 Mar 2021, Andy Shevchenko wrote:
> 
> > Instead of open coding pci_p2sb_bar() functionality we are going to
> > use generic library for that. There one more user of it is coming.
> > 
> > Besides cleaning up it fixes a potential issue if, by some reason,
> > SPI bar is 64-bit.
> 
> Probably worth cleaning up the English in both these sections.
> 
>  Instead of open coding pci_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.

Thanks!

> Also worth briefly describing what that issue is I think.

Current code works if and only if the PCI BAR of the hidden device is inside 4G
address space. In case firmware decides to go above 4G, we will get a wrong
address.

Does it sound good enough?

> > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > ---
> >  drivers/mfd/Kconfig   |  1 +
> >  drivers/mfd/lpc_ich.c | 20 ++++++--------------
> >  2 files changed, 7 insertions(+), 14 deletions(-)
> 
> Code looks fine:
> 
> For my own reference (apply this as-is to your sign-off block):
> 
>   Acked-for-MFD-by: Lee Jones <lee.jones@linaro.org>

Thanks for reviewing this series, can you have a look at the earlier sent [1]
and [2]? First one has a regression fix.

[1]: https://lore.kernel.org/lkml/20210302135620.89958-1-andriy.shevchenko@linux.intel.com/T/#u
[2]: https://lore.kernel.org/lkml/20210301144222.31290-1-andriy.shevchenko@linux.intel.com/T/#u

-- 
With Best Regards,
Andy Shevchenko



^ permalink raw reply	[flat|nested] 54+ messages in thread

* Re: [PATCH v1 5/7] mfd: lpc_ich: Switch to generic pci_p2sb_bar()
  2021-03-10 12:05     ` Andy Shevchenko
@ 2021-03-10 12:57       ` Lee Jones
  0 siblings, 0 replies; 54+ messages in thread
From: Lee Jones @ 2021-03-10 12:57 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: Wolfram Sang, Jean Delvare, Tan Jui Nee, Jim Quinlan,
	Jonathan Yong, Bjorn Helgaas, linux-kernel, linux-i2c, linux-pci,
	Jean Delvare, Peter Tyser, hdegoede, henning.schild

On Wed, 10 Mar 2021, Andy Shevchenko wrote:

> On Wed, Mar 10, 2021 at 10:35:39AM +0000, Lee Jones wrote:
> > On Mon, 08 Mar 2021, Andy Shevchenko wrote:
> > 
> > > Instead of open coding pci_p2sb_bar() functionality we are going to
> > > use generic library for that. There one more user of it is coming.
> > > 
> > > Besides cleaning up it fixes a potential issue if, by some reason,
> > > SPI bar is 64-bit.
> > 
> > Probably worth cleaning up the English in both these sections.
> > 
> >  Instead of open coding pci_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.
> 
> Thanks!
> 
> > Also worth briefly describing what that issue is I think.
> 
> Current code works if and only if the PCI BAR of the hidden device is inside 4G
> address space. In case firmware decides to go above 4G, we will get a wrong
> address.
> 
> Does it sound good enough?

Yes, that explains it, thanks.

> > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > > ---
> > >  drivers/mfd/Kconfig   |  1 +
> > >  drivers/mfd/lpc_ich.c | 20 ++++++--------------
> > >  2 files changed, 7 insertions(+), 14 deletions(-)
> > 
> > Code looks fine:
> > 
> > For my own reference (apply this as-is to your sign-off block):
> > 
> >   Acked-for-MFD-by: Lee Jones <lee.jones@linaro.org>
> 
> Thanks for reviewing this series, can you have a look at the earlier sent [1]
> and [2]? First one has a regression fix.

Yes, thanks for the nudge.  These were not in my TODO list.

> [1]: https://lore.kernel.org/lkml/20210302135620.89958-1-andriy.shevchenko@linux.intel.com/T/#u
> [2]: https://lore.kernel.org/lkml/20210301144222.31290-1-andriy.shevchenko@linux.intel.com/T/#u
> 

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

^ permalink raw reply	[flat|nested] 54+ messages in thread

* Re: [PATCH v1 7/7] i2c: i801: convert to use common P2SB accessor
  2021-03-08 12:20 ` [PATCH v1 7/7] i2c: i801: convert to use common P2SB accessor Andy Shevchenko
@ 2021-03-10 14:51   ` Jean Delvare
  2021-12-21 15:08     ` Andy Shevchenko
  0 siblings, 1 reply; 54+ messages in thread
From: Jean Delvare @ 2021-03-10 14:51 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: Wolfram Sang, Lee Jones, Tan Jui Nee, Jim Quinlan, Jonathan Yong,
	Bjorn Helgaas, linux-kernel, linux-i2c, linux-pci, Peter Tyser,
	hdegoede, henning.schild

Hi Andy,

On Mon,  8 Mar 2021 14:20:20 +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.

I like the idea. Just two things...

> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  drivers/i2c/busses/Kconfig    |  1 +
>  drivers/i2c/busses/i2c-i801.c | 40 ++++++++---------------------------
>  drivers/pci/pci-p2sb.c        |  6 ++++++
>  3 files changed, 16 insertions(+), 31 deletions(-)
> 
> diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
> index 05ebf7546e3f..ffd3007f888c 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 PCI_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 4acee6f9e5a3..23b43de9786a 100644
> --- a/drivers/i2c/busses/i2c-i801.c
> +++ b/drivers/i2c/busses/i2c-i801.c
> @@ -90,6 +90,7 @@
>  #include <linux/interrupt.h>
>  #include <linux/module.h>
>  #include <linux/pci.h>
> +#include <linux/pci-p2sb.h>
>  #include <linux/kernel.h>
>  #include <linux/stddef.h>
>  #include <linux/delay.h>
> @@ -136,7 +137,6 @@
>  #define TCOBASE		0x050
>  #define TCOCTL		0x054
>  
> -#define SBREG_BAR		0x10
>  #define SBREG_SMBCTRL		0xc6000c
>  #define SBREG_SMBCTRL_DNV	0xcf000c
>  
> @@ -1524,52 +1524,30 @@ static const struct itco_wdt_platform_data spt_tco_platform_data = {
>  	.version = 4,
>  };
>  
> -static DEFINE_SPINLOCK(p2sb_spinlock);
> -
>  static struct platform_device *
>  i801_add_tco_spt(struct i801_priv *priv, struct pci_dev *pci_dev,
>  		 struct resource *tco_res)
>  {
>  	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.
> +	 * bridge (P2SB).
>  	 */
> -	spin_lock(&p2sb_spinlock);
>  
>  	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);
> -	spin_unlock(&p2sb_spinlock);
> -
>  	res = &tco_res[1];
> +	ret = pci_p2sb_bar(pci_dev, devfn, 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->end = res->start + 3;
> -	res->flags = IORESOURCE_MEM;
> +		res->start += SBREG_SMBCTRL;

I can't see why you no longer set res->end and res->flags here. I can
imagine that pci_p2sb_bar() may have set the flags for us, but not that
->end is still correct after you fixed up ->start. Am I missing
something?

>  
>  	return platform_device_register_resndata(&pci_dev->dev, "iTCO_wdt", -1,
>  					tco_res, 2, &spt_tco_platform_data,
> diff --git a/drivers/pci/pci-p2sb.c b/drivers/pci/pci-p2sb.c
> index 68d7dad48cdb..7f6bc7d4482a 100644
> --- a/drivers/pci/pci-p2sb.c
> +++ b/drivers/pci/pci-p2sb.c
> @@ -22,6 +22,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)),
>  	{}
>  };
>  

Any reason why this is added in this patch instead of [3/7] (PCI: New
Primary to Sideband (P2SB) bridge support library)?

-- 
Jean Delvare
SUSE L3 Support

^ permalink raw reply	[flat|nested] 54+ messages in thread

* Re: [PATCH v1 1/7] PCI: Introduce pci_bus_*() printing macros when device is not available
  2021-03-08 12:20 ` [PATCH v1 1/7] PCI: Introduce pci_bus_*() printing macros when device is not available Andy Shevchenko
@ 2021-03-10 14:57   ` Jean Delvare
  0 siblings, 0 replies; 54+ messages in thread
From: Jean Delvare @ 2021-03-10 14:57 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: Wolfram Sang, Lee Jones, Tan Jui Nee, Jim Quinlan, Jonathan Yong,
	Bjorn Helgaas, linux-kernel, linux-i2c, linux-pci, Peter Tyser,
	hdegoede, henning.schild

On Mon,  8 Mar 2021 14:20:14 +0200, Andy Shevchenko wrote:
> 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>
> ---
>  drivers/pci/probe.c | 12 +++---------
>  include/linux/pci.h |  9 +++++++++
>  2 files changed, 12 insertions(+), 9 deletions(-)
> (...)

Nice.

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

(If you introduced a _debug flavor of that, it could probably be used in
drivers/pci/hotplug/pciehp_hpc.c.)

-- 
Jean Delvare
SUSE L3 Support

^ permalink raw reply	[flat|nested] 54+ messages in thread

* Re: [PATCH v1 2/7] PCI: Convert __pci_read_base() to __pci_bus_read_base()
  2021-03-08 12:20 ` [PATCH v1 2/7] PCI: Convert __pci_read_base() to __pci_bus_read_base() Andy Shevchenko
@ 2021-03-10 17:14   ` Christoph Hellwig
  0 siblings, 0 replies; 54+ messages in thread
From: Christoph Hellwig @ 2021-03-10 17:14 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: Wolfram Sang, Jean Delvare, Lee Jones, Tan Jui Nee, Jim Quinlan,
	Jonathan Yong, Bjorn Helgaas, linux-kernel, linux-i2c, linux-pci,
	Jean Delvare, Peter Tyser, hdegoede, henning.schild

> +static inline
>  int __pci_read_base(struct pci_dev *dev, enum pci_bar_type type,
> +		    struct resource *res, unsigned int reg)

This looks weird.  Normal kernel style would be:

static inline int __pci_read_base(struct pci_dev *dev, enum pci_bar_type type,
		struct resource *res, unsigned int reg)

or

static inline int
__pci_read_base(struct pci_dev *dev, enum pci_bar_type type,

that being said, there seems to be no good agument to even make this
and inline function.

> +	return __pci_bus_read_base(dev->bus, dev->devfn, type, res, reg, dev->mmio_always_on);

Please avoid pointless overly long lines.

^ permalink raw reply	[flat|nested] 54+ messages in thread

* Re: [rfc, PATCH v1 0/7] PCI: introduce p2sb helper
  2021-03-08 12:20 [rfc, PATCH v1 0/7] PCI: introduce p2sb helper Andy Shevchenko
                   ` (6 preceding siblings ...)
  2021-03-08 12:20 ` [PATCH v1 7/7] i2c: i801: convert to use common P2SB accessor Andy Shevchenko
@ 2021-03-13  9:25 ` Henning Schild
  2021-06-10  9:02 ` Henning Schild
  2021-11-26 15:43 ` Andy Shevchenko
  9 siblings, 0 replies; 54+ messages in thread
From: Henning Schild @ 2021-03-13  9:25 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: Wolfram Sang, Jean Delvare, Lee Jones, Tan Jui Nee, Jim Quinlan,
	Jonathan Yong, Bjorn Helgaas, linux-kernel, linux-i2c, linux-pci,
	Jean Delvare, Peter Tyser, hdegoede

Am Mon, 8 Mar 2021 14:20:13 +0200
schrieb Andy Shevchenko <andriy.shevchenko@linux.intel.com>:

> There are a few users and even at least one more is coming
> that would like to utilize p2sb mechanisms like hide/unhide
> a device from PCI configuration space.

Tried this for my usecase and can confirm it to work as expected.

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

Henning

> 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.
> 
> Please, comment on the approach and individual patches.
> 
> (Since it's cross subsystem, the PCI seems like a main one and
>  I think it makes sense to route it thru it with immutable tag
>  or branch provided for the others).
> 
> Andy Shevchenko (5):
>   PCI: Introduce pci_bus_*() printing macros when device is not
>     available
>   PCI: Convert __pci_read_base() to __pci_bus_read_base()
>   mfd: lpc_ich: Factor out lpc_ich_enable_spi_write()
>   mfd: lpc_ich: Switch to generic pci_p2sb_bar()
>   i2c: i801: convert to use common P2SB accessor
> 
> Jonathan Yong (1):
>   PCI: New Primary to Sideband (P2SB) bridge support library
> 
> 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 |  40 +++-------
>  drivers/mfd/Kconfig           |   1 +
>  drivers/mfd/lpc_ich.c         | 135
> +++++++++++++++++++++++++++++----- drivers/pci/Kconfig           |
> 8 ++ drivers/pci/Makefile          |   1 +
>  drivers/pci/pci-p2sb.c        |  89 ++++++++++++++++++++++
>  drivers/pci/pci.h             |  13 +++-
>  drivers/pci/probe.c           |  81 ++++++++++----------
>  include/linux/pci-p2sb.h      |  28 +++++++
>  include/linux/pci.h           |   9 +++
>  11 files changed, 313 insertions(+), 93 deletions(-)
>  create mode 100644 drivers/pci/pci-p2sb.c
>  create mode 100644 include/linux/pci-p2sb.h
> 


^ permalink raw reply	[flat|nested] 54+ messages in thread

* Re: [PATCH v1 3/7] PCI: New Primary to Sideband (P2SB) bridge support library
  2021-03-08 12:20 ` [PATCH v1 3/7] PCI: New Primary to Sideband (P2SB) bridge support library Andy Shevchenko
  2021-03-08 18:52   ` Bjorn Helgaas
@ 2021-03-13  9:45   ` Henning Schild
  2021-04-01 15:43     ` Andy Shevchenko
  1 sibling, 1 reply; 54+ messages in thread
From: Henning Schild @ 2021-03-13  9:45 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: Wolfram Sang, Jean Delvare, Lee Jones, Tan Jui Nee, Jim Quinlan,
	Jonathan Yong, Bjorn Helgaas, linux-kernel, linux-i2c, linux-pci,
	Jean Delvare, Peter Tyser, hdegoede

Am Mon, 8 Mar 2021 14:20:16 +0200
schrieb Andy Shevchenko <andriy.shevchenko@linux.intel.com>:

> From: Jonathan Yong <jonathan.yong@intel.com>
> 
> There is already one and at least one more user is coming which
> requires an access to Primary to Sideband bridge (P2SB) in order to
> get IO or MMIO bar hidden by BIOS. Create a library to access P2SB
> for x86 devices.
> 
> 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/pci/Kconfig      |  8 ++++
>  drivers/pci/Makefile     |  1 +
>  drivers/pci/pci-p2sb.c   | 83
> ++++++++++++++++++++++++++++++++++++++++ include/linux/pci-p2sb.h |
> 28 ++++++++++++++ 4 files changed, 120 insertions(+)
>  create mode 100644 drivers/pci/pci-p2sb.c
>  create mode 100644 include/linux/pci-p2sb.h
> 
> diff --git a/drivers/pci/Kconfig b/drivers/pci/Kconfig
> index 0c473d75e625..740e5b30d6fd 100644
> --- a/drivers/pci/Kconfig
> +++ b/drivers/pci/Kconfig
> @@ -252,6 +252,14 @@ config PCIE_BUS_PEER2PEER
>  
>  endchoice
>  
> +config PCI_P2SB
> +	bool "Primary to Sideband (P2SB) bridge access support"
> +	depends on PCI && X86
> +	help
> +	  The Primary to Sideband 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.
> +
>  source "drivers/pci/hotplug/Kconfig"
>  source "drivers/pci/controller/Kconfig"
>  source "drivers/pci/endpoint/Kconfig"
> diff --git a/drivers/pci/Makefile b/drivers/pci/Makefile
> index d62c4ac4ae1b..eee8d5dda7d9 100644
> --- a/drivers/pci/Makefile
> +++ b/drivers/pci/Makefile
> @@ -23,6 +23,7 @@ obj-$(CONFIG_PCI_IOV)		+= iov.o
>  obj-$(CONFIG_PCI_BRIDGE_EMUL)	+= pci-bridge-emul.o
>  obj-$(CONFIG_PCI_LABEL)		+= pci-label.o
>  obj-$(CONFIG_X86_INTEL_MID)	+= pci-mid.o
> +obj-$(CONFIG_PCI_P2SB)		+= pci-p2sb.o
>  obj-$(CONFIG_PCI_SYSCALL)	+= syscall.o
>  obj-$(CONFIG_PCI_STUB)		+= pci-stub.o
>  obj-$(CONFIG_PCI_PF_STUB)	+= pci-pf-stub.o
> diff --git a/drivers/pci/pci-p2sb.c b/drivers/pci/pci-p2sb.c
> new file mode 100644
> index 000000000000..68d7dad48cdb
> --- /dev/null
> +++ b/drivers/pci/pci-p2sb.c
> @@ -0,0 +1,83 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Primary to Sideband bridge (P2SB) 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-p2sb.h>
> +
> +#include <asm/cpu_device_id.h>
> +#include <asm/intel-family.h>
> +
> +#include "pci.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 pci_p2sb_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;
> +}
> +
> +/**
> + * pci_p2sb_bar - Get Primary to Sideband bridge (P2SB) device BAR
> + * @pdev:	PCI device to get a PCI bus to communicate with
> + * @devfn:	PCI slot and function to communicate with
> + * @mem:	memory resource to be filled in

Do we really need that many arguments to it?

Before i had, in a platform driver that never had its own pci_dev or bus

  res->start = simatic_ipc_get_membase0(PCI_DEVFN(13, 0));
  if (res-start == 0)
    return -ENODEV;

So helper only asked for the devfn, returned base and no dedicated
error code.

With this i need

  struct pci_bus *bus = pci_find_bus(0, 0);
  struct pci_dev *pci_dev = bus->self;
  unsigned int magic_i_do_not_want =  PCI_DEVFN(13, 0);

> + * 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.
> + *
> + * 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 pci_p2sb_bar(struct pci_dev *pdev, unsigned int devfn, struct
> resource *mem) +{
> +	struct pci_bus *bus = pdev->bus;

if (!pdev)
	bus = pci_find_bus(0, 0);

Or can we drop the whole arg?

> +	unsigned int df;
> +	int ret;
> +
> +	/* Get devfn for P2SB device itself */
> +	ret = pci_p2sb_devfn(&df);
> +	if (ret)
> +		return ret;

if (!devfn)
	devfn = df;

I guess that second devfn is for devices behind that bridge. So
unhiding it might reveal several devices? But when caring about that
p2sb do i really need to know its devfn. If so i would like to get

EXPORT_SYMBOL(pci_p2sb_devfn);

regards,
Henning

> +
> +	pci_lock_rescan_remove();
> +
> +	/* Unhide the P2SB device */
> +	pci_bus_write_config_byte(bus, df, 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, df, P2SBC_HIDE_BYTE,
> P2SBC_HIDE_BIT); +
> +	pci_unlock_rescan_remove();
> +
> +	pci_bus_info(bus, devfn, "BAR: %pR\n", mem);
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(pci_p2sb_bar);
> diff --git a/include/linux/pci-p2sb.h b/include/linux/pci-p2sb.h
> new file mode 100644
> index 000000000000..15dd42737c84
> --- /dev/null
> +++ b/include/linux/pci-p2sb.h
> @@ -0,0 +1,28 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * Primary to Sideband bridge (P2SB) access support
> + */
> +
> +#ifndef _PCI_P2SB_H
> +#define _PCI_P2SB_H
> +
> +#include <linux/errno.h>
> +
> +struct pci_dev;
> +struct resource;
> +
> +#if IS_BUILTIN(CONFIG_PCI_P2SB)
> +
> +int pci_p2sb_bar(struct pci_dev *pdev, unsigned int devfn, struct
> resource *mem); +
> +#else /* CONFIG_PCI_P2SB is not set */
> +
> +static inline
> +int pci_p2sb_bar(struct pci_dev *pdev, unsigned int devfn, struct
> resource *mem) +{
> +	return -ENODEV;
> +}
> +
> +#endif /* CONFIG_PCI_P2SB */
> +
> +#endif /* _PCI_P2SB_H */


^ permalink raw reply	[flat|nested] 54+ messages in thread

* Re: [PATCH v1 3/7] PCI: New Primary to Sideband (P2SB) bridge support library
  2021-03-13  9:45   ` Henning Schild
@ 2021-04-01 15:43     ` Andy Shevchenko
  2021-04-01 18:06       ` Mika Westerberg
  0 siblings, 1 reply; 54+ messages in thread
From: Andy Shevchenko @ 2021-04-01 15:43 UTC (permalink / raw)
  To: Henning Schild
  Cc: Wolfram Sang, Jean Delvare, Lee Jones, Tan Jui Nee, Jim Quinlan,
	Jonathan Yong, Bjorn Helgaas, linux-kernel, linux-i2c, linux-pci,
	Jean Delvare, Peter Tyser, hdegoede, Mika Westerberg

On Sat, Mar 13, 2021 at 10:45:57AM +0100, Henning Schild wrote:
> Am Mon, 8 Mar 2021 14:20:16 +0200
> schrieb Andy Shevchenko <andriy.shevchenko@linux.intel.com>:

...

> > + * pci_p2sb_bar - Get Primary to Sideband bridge (P2SB) device BAR
> > + * @pdev:	PCI device to get a PCI bus to communicate with
> > + * @devfn:	PCI slot and function to communicate with
> > + * @mem:	memory resource to be filled in
> 
> Do we really need that many arguments to it?
> 
> Before i had, in a platform driver that never had its own pci_dev or bus
> 
>   res->start = simatic_ipc_get_membase0(PCI_DEVFN(13, 0));
>   if (res-start == 0)
>     return -ENODEV;
> 
> So helper only asked for the devfn, returned base and no dedicated
> error code.
> 
> With this i need
> 
>   struct pci_bus *bus = pci_find_bus(0, 0);
>   struct pci_dev *pci_dev = bus->self;
>   unsigned int magic_i_do_not_want =  PCI_DEVFN(13, 0);

What confuses me is the use for SPI NOR controller on Broxton. And I think
we actually can indeed hide all this under the hood by exposing P2SB to the OS.

Mika, what do you think?

> I guess that second devfn is for devices behind that bridge. So
> unhiding it might reveal several devices?

Good question. I need a device where actually this happens (hidden P2SB stuff
with let's say SPI NOR there) to see how it looks like in such case. I only
understood the GPIO part. But I'm not so sure anymore.

> But when caring about that
> p2sb do i really need to know its devfn. If so i would like to get

-- 
With Best Regards,
Andy Shevchenko



^ permalink raw reply	[flat|nested] 54+ messages in thread

* Re: [PATCH v1 3/7] PCI: New Primary to Sideband (P2SB) bridge support library
  2021-03-09  8:42         ` Henning Schild
@ 2021-04-01 15:45           ` Andy Shevchenko
  2021-04-01 16:42             ` Bjorn Helgaas
  2021-04-02 13:09           ` Enrico Weigelt, metux IT consult
  1 sibling, 1 reply; 54+ messages in thread
From: Andy Shevchenko @ 2021-04-01 15:45 UTC (permalink / raw)
  To: Henning Schild
  Cc: Bjorn Helgaas, Wolfram Sang, Jean Delvare, Lee Jones,
	Tan Jui Nee, Jim Quinlan, Jonathan Yong, Bjorn Helgaas,
	linux-kernel, linux-i2c, linux-pci, Jean Delvare, Peter Tyser,
	hdegoede

On Tue, Mar 09, 2021 at 09:42:52AM +0100, Henning Schild wrote:
> Am Mon, 8 Mar 2021 19:42:21 -0600
> schrieb Bjorn Helgaas <helgaas@kernel.org>:
> > On Mon, Mar 08, 2021 at 09:16:50PM +0200, Andy Shevchenko wrote:
> > > On Mon, Mar 08, 2021 at 12:52:12PM -0600, Bjorn Helgaas wrote:  
> > > > On Mon, Mar 08, 2021 at 02:20:16PM +0200, Andy Shevchenko wrote:  

...

> > > > > +	/* Read the first BAR of the device in question */
> > > > > +	__pci_bus_read_base(bus, devfn, pci_bar_unknown, mem,
> > > > > PCI_BASE_ADDRESS_0, true);  
> > > > 
> > > > I don't get this.  Apparently this normally hidden device is
> > > > consuming PCI address space.  The PCI core needs to know about
> > > > this.  If it doesn't, the PCI core may assign this space to
> > > > another device.  
> > > 
> > > Right, it returns all 1:s to any request so PCI core *thinks* it's
> > > plugged off (like D3cold or so).  
> > 
> > I'm asking about the MMIO address space.  The BAR is a register in
> > config space.  AFAICT, clearing P2SBC_HIDE_BYTE makes that BAR
> > visible.  The BAR describes a region of PCI address space.  It looks
> > like setting P2SBC_HIDE_BIT makes the BAR disappear from config space,
> > but it sounds like the PCI address space *described* by the BAR is
> > still claimed by the device.  If the device didn't respond to that
> > MMIO space, you would have no reason to read the BAR at all.
> > 
> > So what keeps the PCI core from assigning that MMIO space to another
> > device?
> 
> The device will respond to MMIO while being hidden. I am afraid nothing
> stops a collision, except for the assumption that the BIOS is always
> right and PCI devices never get remapped. But just guessing here.
> 
> I have seen devices with coreboot having the P2SB visible, and most
> likely relocatable. Making it visible in Linux and not hiding it again
> might work, but probably only as long as Linux will not relocate it.
> Which i am afraid might seriously upset the BIOS, depending on what a
> device does with those GPIOs and which parts are implemented in the
> BIOS.

So the question is, do we have knobs in PCI core to mark device fixes in terms
of BARs, no relocation must be applied, no other devices must have the region?

-- 
With Best Regards,
Andy Shevchenko



^ permalink raw reply	[flat|nested] 54+ messages in thread

* Re: [PATCH v1 3/7] PCI: New Primary to Sideband (P2SB) bridge support library
  2021-04-01 15:45           ` Andy Shevchenko
@ 2021-04-01 16:42             ` Bjorn Helgaas
  2021-04-01 18:23               ` Andy Shevchenko
  0 siblings, 1 reply; 54+ messages in thread
From: Bjorn Helgaas @ 2021-04-01 16:42 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: Henning Schild, Wolfram Sang, Jean Delvare, Lee Jones,
	Tan Jui Nee, Jim Quinlan, Jonathan Yong, Bjorn Helgaas,
	linux-kernel, linux-i2c, linux-pci, Jean Delvare, Peter Tyser,
	hdegoede

On Thu, Apr 01, 2021 at 06:45:02PM +0300, Andy Shevchenko wrote:
> On Tue, Mar 09, 2021 at 09:42:52AM +0100, Henning Schild wrote:
> > Am Mon, 8 Mar 2021 19:42:21 -0600
> > schrieb Bjorn Helgaas <helgaas@kernel.org>:
> > > On Mon, Mar 08, 2021 at 09:16:50PM +0200, Andy Shevchenko wrote:
> > > > On Mon, Mar 08, 2021 at 12:52:12PM -0600, Bjorn Helgaas wrote:  
> > > > > On Mon, Mar 08, 2021 at 02:20:16PM +0200, Andy Shevchenko wrote:  
> 
> ...
> 
> > > > > > +	/* Read the first BAR of the device in question */
> > > > > > +	__pci_bus_read_base(bus, devfn, pci_bar_unknown, mem,
> > > > > > PCI_BASE_ADDRESS_0, true);  
> > > > > 
> > > > > I don't get this.  Apparently this normally hidden device is
> > > > > consuming PCI address space.  The PCI core needs to know
> > > > > about this.  If it doesn't, the PCI core may assign this
> > > > > space to another device.  
> > > > 
> > > > Right, it returns all 1:s to any request so PCI core *thinks*
> > > > it's plugged off (like D3cold or so).  
> > > 
> > > I'm asking about the MMIO address space.  The BAR is a register
> > > in config space.  AFAICT, clearing P2SBC_HIDE_BYTE makes that
> > > BAR visible.  The BAR describes a region of PCI address space.
> > > It looks like setting P2SBC_HIDE_BIT makes the BAR disappear
> > > from config space, but it sounds like the PCI address space
> > > *described* by the BAR is still claimed by the device.  If the
> > > device didn't respond to that MMIO space, you would have no
> > > reason to read the BAR at all.
> > > 
> > > So what keeps the PCI core from assigning that MMIO space to
> > > another device?
> > 
> > The device will respond to MMIO while being hidden. I am afraid
> > nothing stops a collision, except for the assumption that the BIOS
> > is always right and PCI devices never get remapped. But just
> > guessing here.
> > 
> > I have seen devices with coreboot having the P2SB visible, and
> > most likely relocatable. Making it visible in Linux and not hiding
> > it again might work, but probably only as long as Linux will not
> > relocate it.  Which i am afraid might seriously upset the BIOS,
> > depending on what a device does with those GPIOs and which parts
> > are implemented in the BIOS.
> 
> So the question is, do we have knobs in PCI core to mark device
> fixes in terms of BARs, no relocation must be applied, no other
> devices must have the region?

I think the closest thing is the IORESOURCE_PCI_FIXED bit that we use
for things that must not be moved.  Generally PCI resources are
associated with a pci_dev, and we set IORESOURCE_PCI_FIXED for BARs,
e.g., dev->resource[n].  We do that for IDE legacy regions (see
LEGACY_IO_RESOURCE), Langwell devices (pci_fixed_bar_fixup()),
"enhanced allocation" (pci_ea_flags()), and some quirks (quirk_io()).

In your case, the device is hidden so it doesn't respond to config
accesses, so there is no pci_dev for it.

Maybe you could do some sort of quirk that allocates its own struct
resource, fills it in, sets IORESOURCE_PCI_FIXED, and does something
similar to pci_claim_resource()?

Bjorn

^ permalink raw reply	[flat|nested] 54+ messages in thread

* Re: [PATCH v1 3/7] PCI: New Primary to Sideband (P2SB) bridge support library
  2021-04-01 15:43     ` Andy Shevchenko
@ 2021-04-01 18:06       ` Mika Westerberg
  2021-04-01 18:22         ` Andy Shevchenko
  0 siblings, 1 reply; 54+ messages in thread
From: Mika Westerberg @ 2021-04-01 18:06 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: Henning Schild, Wolfram Sang, Jean Delvare, Lee Jones,
	Tan Jui Nee, Jim Quinlan, Jonathan Yong, Bjorn Helgaas,
	linux-kernel, linux-i2c, linux-pci, Jean Delvare, Peter Tyser,
	hdegoede

On Thu, Apr 01, 2021 at 06:43:11PM +0300, Andy Shevchenko wrote:
> On Sat, Mar 13, 2021 at 10:45:57AM +0100, Henning Schild wrote:
> > Am Mon, 8 Mar 2021 14:20:16 +0200
> > schrieb Andy Shevchenko <andriy.shevchenko@linux.intel.com>:
> 
> ...
> 
> > > + * pci_p2sb_bar - Get Primary to Sideband bridge (P2SB) device BAR
> > > + * @pdev:	PCI device to get a PCI bus to communicate with
> > > + * @devfn:	PCI slot and function to communicate with
> > > + * @mem:	memory resource to be filled in
> > 
> > Do we really need that many arguments to it?
> > 
> > Before i had, in a platform driver that never had its own pci_dev or bus
> > 
> >   res->start = simatic_ipc_get_membase0(PCI_DEVFN(13, 0));
> >   if (res-start == 0)
> >     return -ENODEV;
> > 
> > So helper only asked for the devfn, returned base and no dedicated
> > error code.
> > 
> > With this i need
> > 
> >   struct pci_bus *bus = pci_find_bus(0, 0);
> >   struct pci_dev *pci_dev = bus->self;
> >   unsigned int magic_i_do_not_want =  PCI_DEVFN(13, 0);
> 
> What confuses me is the use for SPI NOR controller on Broxton. And I think
> we actually can indeed hide all this under the hood by exposing P2SB to the OS.
> 
> Mika, what do you think?

Not sure I follow. Do you mean we force unhide P2SB and then bind (MFD)
driver to that?

^ permalink raw reply	[flat|nested] 54+ messages in thread

* Re: [PATCH v1 3/7] PCI: New Primary to Sideband (P2SB) bridge support library
  2021-04-01 18:06       ` Mika Westerberg
@ 2021-04-01 18:22         ` Andy Shevchenko
  2021-04-01 18:32           ` Mika Westerberg
  0 siblings, 1 reply; 54+ messages in thread
From: Andy Shevchenko @ 2021-04-01 18:22 UTC (permalink / raw)
  To: Mika Westerberg
  Cc: Henning Schild, Wolfram Sang, Jean Delvare, Lee Jones,
	Tan Jui Nee, Jim Quinlan, Jonathan Yong, Bjorn Helgaas,
	linux-kernel, linux-i2c, linux-pci, Jean Delvare, Peter Tyser,
	hdegoede

On Thu, Apr 01, 2021 at 09:06:17PM +0300, Mika Westerberg wrote:
> On Thu, Apr 01, 2021 at 06:43:11PM +0300, Andy Shevchenko wrote:
> > On Sat, Mar 13, 2021 at 10:45:57AM +0100, Henning Schild wrote:
> > > Am Mon, 8 Mar 2021 14:20:16 +0200
> > > schrieb Andy Shevchenko <andriy.shevchenko@linux.intel.com>:
> > 
> > ...
> > 
> > > > + * pci_p2sb_bar - Get Primary to Sideband bridge (P2SB) device BAR
> > > > + * @pdev:	PCI device to get a PCI bus to communicate with
> > > > + * @devfn:	PCI slot and function to communicate with
> > > > + * @mem:	memory resource to be filled in
> > > 
> > > Do we really need that many arguments to it?
> > > 
> > > Before i had, in a platform driver that never had its own pci_dev or bus
> > > 
> > >   res->start = simatic_ipc_get_membase0(PCI_DEVFN(13, 0));
> > >   if (res-start == 0)
> > >     return -ENODEV;
> > > 
> > > So helper only asked for the devfn, returned base and no dedicated
> > > error code.
> > > 
> > > With this i need
> > > 
> > >   struct pci_bus *bus = pci_find_bus(0, 0);
> > >   struct pci_dev *pci_dev = bus->self;
> > >   unsigned int magic_i_do_not_want =  PCI_DEVFN(13, 0);
> > 
> > What confuses me is the use for SPI NOR controller on Broxton. And I think
> > we actually can indeed hide all this under the hood by exposing P2SB to the OS.
> > 
> > Mika, what do you think?
> 
> Not sure I follow. Do you mean we force unhide P2SB and then bind (MFD)
> driver to that?

Not MFD, SPI NOR (if I understood correctly the code in MFD driver for SPI NOR
in regards to P2SB case).

-- 
With Best Regards,
Andy Shevchenko



^ permalink raw reply	[flat|nested] 54+ messages in thread

* Re: [PATCH v1 3/7] PCI: New Primary to Sideband (P2SB) bridge support library
  2021-04-01 16:42             ` Bjorn Helgaas
@ 2021-04-01 18:23               ` Andy Shevchenko
  2021-04-01 18:44                 ` Bjorn Helgaas
  0 siblings, 1 reply; 54+ messages in thread
From: Andy Shevchenko @ 2021-04-01 18:23 UTC (permalink / raw)
  To: Bjorn Helgaas
  Cc: Henning Schild, Wolfram Sang, Jean Delvare, Lee Jones,
	Tan Jui Nee, Jim Quinlan, Jonathan Yong, Bjorn Helgaas,
	linux-kernel, linux-i2c, linux-pci, Jean Delvare, Peter Tyser,
	hdegoede

On Thu, Apr 01, 2021 at 11:42:56AM -0500, Bjorn Helgaas wrote:
> On Thu, Apr 01, 2021 at 06:45:02PM +0300, Andy Shevchenko wrote:
> > On Tue, Mar 09, 2021 at 09:42:52AM +0100, Henning Schild wrote:
> > > Am Mon, 8 Mar 2021 19:42:21 -0600
> > > schrieb Bjorn Helgaas <helgaas@kernel.org>:
> > > > On Mon, Mar 08, 2021 at 09:16:50PM +0200, Andy Shevchenko wrote:
> > > > > On Mon, Mar 08, 2021 at 12:52:12PM -0600, Bjorn Helgaas wrote:  
> > > > > > On Mon, Mar 08, 2021 at 02:20:16PM +0200, Andy Shevchenko wrote:  
> > 
> > ...
> > 
> > > > > > > +	/* Read the first BAR of the device in question */
> > > > > > > +	__pci_bus_read_base(bus, devfn, pci_bar_unknown, mem,
> > > > > > > PCI_BASE_ADDRESS_0, true);  
> > > > > > 
> > > > > > I don't get this.  Apparently this normally hidden device is
> > > > > > consuming PCI address space.  The PCI core needs to know
> > > > > > about this.  If it doesn't, the PCI core may assign this
> > > > > > space to another device.  
> > > > > 
> > > > > Right, it returns all 1:s to any request so PCI core *thinks*
> > > > > it's plugged off (like D3cold or so).  
> > > > 
> > > > I'm asking about the MMIO address space.  The BAR is a register
> > > > in config space.  AFAICT, clearing P2SBC_HIDE_BYTE makes that
> > > > BAR visible.  The BAR describes a region of PCI address space.
> > > > It looks like setting P2SBC_HIDE_BIT makes the BAR disappear
> > > > from config space, but it sounds like the PCI address space
> > > > *described* by the BAR is still claimed by the device.  If the
> > > > device didn't respond to that MMIO space, you would have no
> > > > reason to read the BAR at all.
> > > > 
> > > > So what keeps the PCI core from assigning that MMIO space to
> > > > another device?
> > > 
> > > The device will respond to MMIO while being hidden. I am afraid
> > > nothing stops a collision, except for the assumption that the BIOS
> > > is always right and PCI devices never get remapped. But just
> > > guessing here.
> > > 
> > > I have seen devices with coreboot having the P2SB visible, and
> > > most likely relocatable. Making it visible in Linux and not hiding
> > > it again might work, but probably only as long as Linux will not
> > > relocate it.  Which i am afraid might seriously upset the BIOS,
> > > depending on what a device does with those GPIOs and which parts
> > > are implemented in the BIOS.
> > 
> > So the question is, do we have knobs in PCI core to mark device
> > fixes in terms of BARs, no relocation must be applied, no other
> > devices must have the region?
> 
> I think the closest thing is the IORESOURCE_PCI_FIXED bit that we use
> for things that must not be moved.  Generally PCI resources are
> associated with a pci_dev, and we set IORESOURCE_PCI_FIXED for BARs,
> e.g., dev->resource[n].  We do that for IDE legacy regions (see
> LEGACY_IO_RESOURCE), Langwell devices (pci_fixed_bar_fixup()),
> "enhanced allocation" (pci_ea_flags()), and some quirks (quirk_io()).
> 
> In your case, the device is hidden so it doesn't respond to config
> accesses, so there is no pci_dev for it.

Yes, and the idea is to unhide it on the early stage.
Would it be possible to quirk it to fix the IO resources?

> Maybe you could do some sort of quirk that allocates its own struct
> resource, fills it in, sets IORESOURCE_PCI_FIXED, and does something
> similar to pci_claim_resource()?

-- 
With Best Regards,
Andy Shevchenko



^ permalink raw reply	[flat|nested] 54+ messages in thread

* Re: [PATCH v1 3/7] PCI: New Primary to Sideband (P2SB) bridge support library
  2021-04-01 18:22         ` Andy Shevchenko
@ 2021-04-01 18:32           ` Mika Westerberg
  2021-07-12 12:13             ` Andy Shevchenko
  0 siblings, 1 reply; 54+ messages in thread
From: Mika Westerberg @ 2021-04-01 18:32 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: Henning Schild, Wolfram Sang, Jean Delvare, Lee Jones,
	Tan Jui Nee, Jim Quinlan, Jonathan Yong, Bjorn Helgaas,
	linux-kernel, linux-i2c, linux-pci, Jean Delvare, Peter Tyser,
	hdegoede

On Thu, Apr 01, 2021 at 09:22:02PM +0300, Andy Shevchenko wrote:
> On Thu, Apr 01, 2021 at 09:06:17PM +0300, Mika Westerberg wrote:
> > On Thu, Apr 01, 2021 at 06:43:11PM +0300, Andy Shevchenko wrote:
> > > On Sat, Mar 13, 2021 at 10:45:57AM +0100, Henning Schild wrote:
> > > > Am Mon, 8 Mar 2021 14:20:16 +0200
> > > > schrieb Andy Shevchenko <andriy.shevchenko@linux.intel.com>:
> > > 
> > > ...
> > > 
> > > > > + * pci_p2sb_bar - Get Primary to Sideband bridge (P2SB) device BAR
> > > > > + * @pdev:	PCI device to get a PCI bus to communicate with
> > > > > + * @devfn:	PCI slot and function to communicate with
> > > > > + * @mem:	memory resource to be filled in
> > > > 
> > > > Do we really need that many arguments to it?
> > > > 
> > > > Before i had, in a platform driver that never had its own pci_dev or bus
> > > > 
> > > >   res->start = simatic_ipc_get_membase0(PCI_DEVFN(13, 0));
> > > >   if (res-start == 0)
> > > >     return -ENODEV;
> > > > 
> > > > So helper only asked for the devfn, returned base and no dedicated
> > > > error code.
> > > > 
> > > > With this i need
> > > > 
> > > >   struct pci_bus *bus = pci_find_bus(0, 0);
> > > >   struct pci_dev *pci_dev = bus->self;
> > > >   unsigned int magic_i_do_not_want =  PCI_DEVFN(13, 0);
> > > 
> > > What confuses me is the use for SPI NOR controller on Broxton. And I think
> > > we actually can indeed hide all this under the hood by exposing P2SB to the OS.
> > > 
> > > Mika, what do you think?
> > 
> > Not sure I follow. Do you mean we force unhide P2SB and then bind (MFD)
> > driver to that?
> 
> Not MFD, SPI NOR (if I understood correctly the code in MFD driver for SPI NOR
> in regards to P2SB case).

I mean a new MFD driver that binds to the P2SB and that one then exposes
the stuff needed by the SPI-NOR driver.

^ permalink raw reply	[flat|nested] 54+ messages in thread

* Re: [PATCH v1 3/7] PCI: New Primary to Sideband (P2SB) bridge support library
  2021-04-01 18:23               ` Andy Shevchenko
@ 2021-04-01 18:44                 ` Bjorn Helgaas
  2021-07-12 12:15                   ` Andy Shevchenko
  0 siblings, 1 reply; 54+ messages in thread
From: Bjorn Helgaas @ 2021-04-01 18:44 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: Henning Schild, Wolfram Sang, Jean Delvare, Lee Jones,
	Tan Jui Nee, Jim Quinlan, Jonathan Yong, Bjorn Helgaas,
	linux-kernel, linux-i2c, linux-pci, Jean Delvare, Peter Tyser,
	hdegoede

On Thu, Apr 01, 2021 at 09:23:04PM +0300, Andy Shevchenko wrote:
> On Thu, Apr 01, 2021 at 11:42:56AM -0500, Bjorn Helgaas wrote:
> > On Thu, Apr 01, 2021 at 06:45:02PM +0300, Andy Shevchenko wrote:
> > > On Tue, Mar 09, 2021 at 09:42:52AM +0100, Henning Schild wrote:
> > > > Am Mon, 8 Mar 2021 19:42:21 -0600
> > > > schrieb Bjorn Helgaas <helgaas@kernel.org>:
> > > > > On Mon, Mar 08, 2021 at 09:16:50PM +0200, Andy Shevchenko wrote:
> > > > > > On Mon, Mar 08, 2021 at 12:52:12PM -0600, Bjorn Helgaas wrote:  
> > > > > > > On Mon, Mar 08, 2021 at 02:20:16PM +0200, Andy Shevchenko wrote:  
> > > 
> > > ...
> > > 
> > > > > > > > +	/* Read the first BAR of the device in question */
> > > > > > > > +	__pci_bus_read_base(bus, devfn, pci_bar_unknown, mem,
> > > > > > > > PCI_BASE_ADDRESS_0, true);  
> > > > > > > 
> > > > > > > I don't get this.  Apparently this normally hidden device is
> > > > > > > consuming PCI address space.  The PCI core needs to know
> > > > > > > about this.  If it doesn't, the PCI core may assign this
> > > > > > > space to another device.  
> > > > > > 
> > > > > > Right, it returns all 1:s to any request so PCI core *thinks*
> > > > > > it's plugged off (like D3cold or so).  
> > > > > 
> > > > > I'm asking about the MMIO address space.  The BAR is a register
> > > > > in config space.  AFAICT, clearing P2SBC_HIDE_BYTE makes that
> > > > > BAR visible.  The BAR describes a region of PCI address space.
> > > > > It looks like setting P2SBC_HIDE_BIT makes the BAR disappear
> > > > > from config space, but it sounds like the PCI address space
> > > > > *described* by the BAR is still claimed by the device.  If the
> > > > > device didn't respond to that MMIO space, you would have no
> > > > > reason to read the BAR at all.
> > > > > 
> > > > > So what keeps the PCI core from assigning that MMIO space to
> > > > > another device?
> > > > 
> > > > The device will respond to MMIO while being hidden. I am afraid
> > > > nothing stops a collision, except for the assumption that the BIOS
> > > > is always right and PCI devices never get remapped. But just
> > > > guessing here.
> > > > 
> > > > I have seen devices with coreboot having the P2SB visible, and
> > > > most likely relocatable. Making it visible in Linux and not hiding
> > > > it again might work, but probably only as long as Linux will not
> > > > relocate it.  Which i am afraid might seriously upset the BIOS,
> > > > depending on what a device does with those GPIOs and which parts
> > > > are implemented in the BIOS.
> > > 
> > > So the question is, do we have knobs in PCI core to mark device
> > > fixes in terms of BARs, no relocation must be applied, no other
> > > devices must have the region?
> > 
> > I think the closest thing is the IORESOURCE_PCI_FIXED bit that we use
> > for things that must not be moved.  Generally PCI resources are
> > associated with a pci_dev, and we set IORESOURCE_PCI_FIXED for BARs,
> > e.g., dev->resource[n].  We do that for IDE legacy regions (see
> > LEGACY_IO_RESOURCE), Langwell devices (pci_fixed_bar_fixup()),
> > "enhanced allocation" (pci_ea_flags()), and some quirks (quirk_io()).
> > 
> > In your case, the device is hidden so it doesn't respond to config
> > accesses, so there is no pci_dev for it.
> 
> Yes, and the idea is to unhide it on the early stage.
> Would it be possible to quirk it to fix the IO resources?

If I read your current patch right, it unhides the device, reads the
BAR, then hides the device again.  I didn't see that it would create a
pci_dev for it.

If you unhide it and then enumerate it normally (and mark the BAR as
IORESOURCE_PCI_FIXED to make sure we never move it), that might work.
Then there should be a pci_dev for it, and it would then show up in
sysfs, lspci, etc.  And we should insert the BAR in iomem_resource, so
we should see it in /proc/iomem and we won't accidentally put
something else on top of it.

> > Maybe you could do some sort of quirk that allocates its own struct
> > resource, fills it in, sets IORESOURCE_PCI_FIXED, and does something
> > similar to pci_claim_resource()?
> 
> -- 
> With Best Regards,
> Andy Shevchenko
> 
> 

^ permalink raw reply	[flat|nested] 54+ messages in thread

* Re: [PATCH v1 3/7] PCI: New Primary to Sideband (P2SB) bridge support library
  2021-03-09  8:42         ` Henning Schild
  2021-04-01 15:45           ` Andy Shevchenko
@ 2021-04-02 13:09           ` Enrico Weigelt, metux IT consult
  2021-04-06 13:40             ` Henning Schild
  1 sibling, 1 reply; 54+ messages in thread
From: Enrico Weigelt, metux IT consult @ 2021-04-02 13:09 UTC (permalink / raw)
  To: Henning Schild, Bjorn Helgaas
  Cc: Andy Shevchenko, Wolfram Sang, Jean Delvare, Lee Jones,
	Tan Jui Nee, Jim Quinlan, Jonathan Yong, Bjorn Helgaas,
	linux-kernel, linux-i2c, linux-pci, Jean Delvare, Peter Tyser,
	hdegoede

On 09.03.21 09:42, Henning Schild wrote:

> The device will respond to MMIO while being hidden. I am afraid nothing
> stops a collision, except for the assumption that the BIOS is always
> right and PCI devices never get remapped. But just guessing here.

What could go wrong if it is remapped, except that this driver would
write to the wrong mmio space ?

If it's unhidden, pci-core should see it and start the usual probing,
right ?


--mtx

-- 
---
Hinweis: unverschlüsselte E-Mails können leicht abgehört und manipuliert
werden ! Für eine vertrauliche Kommunikation senden Sie bitte ihren
GPG/PGP-Schlüssel zu.
---
Enrico Weigelt, metux IT consult
Free software and Linux embedded engineering
info@metux.net -- +49-151-27565287

^ permalink raw reply	[flat|nested] 54+ messages in thread

* Re: [PATCH v1 3/7] PCI: New Primary to Sideband (P2SB) bridge support library
  2021-04-02 13:09           ` Enrico Weigelt, metux IT consult
@ 2021-04-06 13:40             ` Henning Schild
  2021-07-12 12:11               ` Andy Shevchenko
  0 siblings, 1 reply; 54+ messages in thread
From: Henning Schild @ 2021-04-06 13:40 UTC (permalink / raw)
  To: Enrico Weigelt, metux IT consult
  Cc: Bjorn Helgaas, Andy Shevchenko, Wolfram Sang, Jean Delvare,
	Lee Jones, Tan Jui Nee, Jim Quinlan, Jonathan Yong,
	Bjorn Helgaas, linux-kernel, linux-i2c, linux-pci, Jean Delvare,
	Peter Tyser, hdegoede

Am Fri, 2 Apr 2021 15:09:12 +0200
schrieb "Enrico Weigelt, metux IT consult" <lkml@metux.net>:

> On 09.03.21 09:42, Henning Schild wrote:
> 
> > The device will respond to MMIO while being hidden. I am afraid
> > nothing stops a collision, except for the assumption that the BIOS
> > is always right and PCI devices never get remapped. But just
> > guessing here.  
> 
> What could go wrong if it is remapped, except that this driver would
> write to the wrong mmio space ?
> 
> If it's unhidden, pci-core should see it and start the usual probing,
> right ?

I have seen this guy exposed to Linux on coreboot machines. No issues.
But i can imagine BIOSs that somehow make use of the device and assume
it wont move. So we would at least need a parameter to allow keeping
that device hidden, or "fixed" in memory.

Henning

> 
> --mtx
> 


^ permalink raw reply	[flat|nested] 54+ messages in thread

* Re: [PATCH v1 6/7] mfd: lpc_ich: Add support for pinctrl in non-ACPI system
  2021-03-08 12:20 ` [PATCH v1 6/7] mfd: lpc_ich: Add support for pinctrl in non-ACPI system Andy Shevchenko
  2021-03-10 10:27   ` Lee Jones
@ 2021-04-12 16:01   ` Henning Schild
  2021-04-12 16:40     ` Henning Schild
  2021-04-12 16:51     ` Andy Shevchenko
  1 sibling, 2 replies; 54+ messages in thread
From: Henning Schild @ 2021-04-12 16:01 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: Wolfram Sang, Jean Delvare, Lee Jones, Tan Jui Nee, Jim Quinlan,
	Jonathan Yong, Bjorn Helgaas, linux-kernel, linux-i2c, linux-pci,
	Jean Delvare, Peter Tyser, hdegoede

Am Mon, 8 Mar 2021 14:20:19 +0200
schrieb Andy Shevchenko <andriy.shevchenko@linux.intel.com>:

> From: Tan Jui Nee <jui.nee.tan@intel.com>
> 
> Add support for non-ACPI systems, such as system that uses
> Advanced Boot Loader (ABL) whereby a platform device has to be created
> in order to bind with pin control and GPIO.
> 
> At the moment, Intel Apollo Lake In-Vehicle Infotainment (IVI) system
> requires a driver to hide and unhide P2SB to lookup P2SB BAR and pass
> the PCI BAR address to GPIO.
> 
> Signed-off-by: Tan Jui Nee <jui.nee.tan@intel.com>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  drivers/mfd/lpc_ich.c | 100
> +++++++++++++++++++++++++++++++++++++++++- 1 file changed, 99
> insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/mfd/lpc_ich.c b/drivers/mfd/lpc_ich.c
> index 8e9bd6813287..959247b6987a 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>
>   *
> @@ -43,6 +44,7 @@
>  #include <linux/acpi.h>
>  #include <linux/pci.h>
>  #include <linux/pci-p2sb.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,73 @@ static struct mfd_cell lpc_ich_gpio_cell = {
>  	.ignore_resource_conflicts = true,
>  };
>  
> +/* Offset data for Apollo Lake GPIO controllers */
> +#define APL_GPIO_SOUTHWEST_OFFSET	0xc00000
> +#define APL_GPIO_SOUTHWEST_SIZE		0x654
> +#define APL_GPIO_NORTHWEST_OFFSET	0xc40000
> +#define APL_GPIO_NORTHWEST_SIZE		0x764
> +#define APL_GPIO_NORTH_OFFSET		0xc50000
> +#define APL_GPIO_NORTH_SIZE		0x76c

drivers/pinctrl/intel/pinctrl-broxton.c:653
BXT_COMMUNITY(0, 77),

> +#define APL_GPIO_WEST_OFFSET		0xc70000
> +#define APL_GPIO_WEST_SIZE		0x674

All these sizes correlate with 4 magic numbers from pinctrl-broxton.

SIZE - 0x500 (pad_base?) - 4 (no clue) / 8

It might be worth basing both numbers on a define and giving the magic
numbers some names.

But all this seems like duplication of pinctrl-broxton, maybe the
pinctrl driver should unhide the p2sb ...

regards,
Henning

> +
> +#define APL_GPIO_NR_DEVICES		4
> +#define APL_GPIO_IRQ			14
> +
> +static struct resource apl_gpio_resources[APL_GPIO_NR_DEVICES][2] = {
> +	{
> +		DEFINE_RES_MEM(APL_GPIO_NORTH_OFFSET,
> APL_GPIO_NORTH_SIZE),
> +		DEFINE_RES_IRQ(APL_GPIO_IRQ),
> +	},
> +	{
> +		DEFINE_RES_MEM(APL_GPIO_NORTHWEST_OFFSET,
> APL_GPIO_NORTHWEST_SIZE),
> +		DEFINE_RES_IRQ(APL_GPIO_IRQ),
> +	},
> +	{
> +		DEFINE_RES_MEM(APL_GPIO_WEST_OFFSET,
> APL_GPIO_WEST_SIZE),
> +		DEFINE_RES_IRQ(APL_GPIO_IRQ),
> +	},
> +	{
> +		DEFINE_RES_MEM(APL_GPIO_SOUTHWEST_OFFSET,
> APL_GPIO_SOUTHWEST_SIZE),
> +		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] =
> {
> +	{
> +		/* North */
> +		.name = "apollolake-pinctrl",
> +		.id = 0,
> +		.num_resources = ARRAY_SIZE(apl_gpio_resources[0]),
> +		.resources = apl_gpio_resources[0],
> +		.ignore_resource_conflicts = true,
> +	},
> +	{
> +		/* NorthWest */
> +		.name = "apollolake-pinctrl",
> +		.id = 1,
> +		.num_resources = ARRAY_SIZE(apl_gpio_resources[1]),
> +		.resources = apl_gpio_resources[1],
> +		.ignore_resource_conflicts = true,
> +	},
> +	{
> +		/* West */
> +		.name = "apollolake-pinctrl",
> +		.id = 2,
> +		.num_resources = ARRAY_SIZE(apl_gpio_resources[2]),
> +		.resources = apl_gpio_resources[2],
> +		.ignore_resource_conflicts = true,
> +	},
> +	{
> +		/* SouthWest */
> +		.name = "apollolake-pinctrl",
> +		.id = 3,
> +		.num_resources = ARRAY_SIZE(apl_gpio_resources[3]),
> +		.resources = apl_gpio_resources[3],
> +		.ignore_resource_conflicts = true,
> +	},
> +};
>  
>  static struct mfd_cell lpc_ich_spi_cell = {
>  	.name = "intel-spi",
> @@ -1082,6 +1151,29 @@ 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;
> +
> +	ret = pci_p2sb_bar(dev, PCI_DEVFN(13, 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)
>  {
> @@ -1198,6 +1290,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)


^ permalink raw reply	[flat|nested] 54+ messages in thread

* Re: [PATCH v1 6/7] mfd: lpc_ich: Add support for pinctrl in non-ACPI system
  2021-04-12 16:01   ` Henning Schild
@ 2021-04-12 16:40     ` Henning Schild
  2021-04-12 16:59       ` Andy Shevchenko
  2021-04-12 16:51     ` Andy Shevchenko
  1 sibling, 1 reply; 54+ messages in thread
From: Henning Schild @ 2021-04-12 16:40 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: Wolfram Sang, Jean Delvare, Lee Jones, Tan Jui Nee, Jim Quinlan,
	Jonathan Yong, Bjorn Helgaas, linux-kernel, linux-i2c, linux-pci,
	Jean Delvare, Peter Tyser, hdegoede

Tan or Andy,

maybe you can point me to a user of that patch. I guess there might be
an out-of-tree driver or userland code on how to use the GPIOs from
there.

Feel free to send directly to me in case it is not published anywhere
and should not yet be on the list, i could just use it for inspiration.
A driver will likely be GPL anyways.

regards,
Henning

Am Mon, 12 Apr 2021 18:01:06 +0200
schrieb Henning Schild <henning.schild@siemens.com>:

> Am Mon, 8 Mar 2021 14:20:19 +0200
> schrieb Andy Shevchenko <andriy.shevchenko@linux.intel.com>:
> 
> > From: Tan Jui Nee <jui.nee.tan@intel.com>
> > 
> > Add support for non-ACPI systems, such as system that uses
> > Advanced Boot Loader (ABL) whereby a platform device has to be
> > created in order to bind with pin control and GPIO.
> > 
> > At the moment, Intel Apollo Lake In-Vehicle Infotainment (IVI)
> > system requires a driver to hide and unhide P2SB to lookup P2SB BAR
> > and pass the PCI BAR address to GPIO.
> > 
> > Signed-off-by: Tan Jui Nee <jui.nee.tan@intel.com>
> > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > ---
> >  drivers/mfd/lpc_ich.c | 100
> > +++++++++++++++++++++++++++++++++++++++++- 1 file changed, 99
> > insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/mfd/lpc_ich.c b/drivers/mfd/lpc_ich.c
> > index 8e9bd6813287..959247b6987a 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>
> >   *
> > @@ -43,6 +44,7 @@
> >  #include <linux/acpi.h>
> >  #include <linux/pci.h>
> >  #include <linux/pci-p2sb.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,73 @@ static struct mfd_cell lpc_ich_gpio_cell = {
> >  	.ignore_resource_conflicts = true,
> >  };
> >  
> > +/* Offset data for Apollo Lake GPIO controllers */
> > +#define APL_GPIO_SOUTHWEST_OFFSET	0xc00000
> > +#define APL_GPIO_SOUTHWEST_SIZE		0x654
> > +#define APL_GPIO_NORTHWEST_OFFSET	0xc40000
> > +#define APL_GPIO_NORTHWEST_SIZE		0x764
> > +#define APL_GPIO_NORTH_OFFSET		0xc50000
> > +#define APL_GPIO_NORTH_SIZE		0x76c  
> 
> drivers/pinctrl/intel/pinctrl-broxton.c:653
> BXT_COMMUNITY(0, 77),
> 
> > +#define APL_GPIO_WEST_OFFSET		0xc70000
> > +#define APL_GPIO_WEST_SIZE		0x674  
> 
> All these sizes correlate with 4 magic numbers from pinctrl-broxton.
> 
> SIZE - 0x500 (pad_base?) - 4 (no clue) / 8
> 
> It might be worth basing both numbers on a define and giving the magic
> numbers some names.
> 
> But all this seems like duplication of pinctrl-broxton, maybe the
> pinctrl driver should unhide the p2sb ...
> 
> regards,
> Henning
> 
> > +
> > +#define APL_GPIO_NR_DEVICES		4
> > +#define APL_GPIO_IRQ			14
> > +
> > +static struct resource apl_gpio_resources[APL_GPIO_NR_DEVICES][2]
> > = {
> > +	{
> > +		DEFINE_RES_MEM(APL_GPIO_NORTH_OFFSET,
> > APL_GPIO_NORTH_SIZE),
> > +		DEFINE_RES_IRQ(APL_GPIO_IRQ),
> > +	},
> > +	{
> > +		DEFINE_RES_MEM(APL_GPIO_NORTHWEST_OFFSET,
> > APL_GPIO_NORTHWEST_SIZE),
> > +		DEFINE_RES_IRQ(APL_GPIO_IRQ),
> > +	},
> > +	{
> > +		DEFINE_RES_MEM(APL_GPIO_WEST_OFFSET,
> > APL_GPIO_WEST_SIZE),
> > +		DEFINE_RES_IRQ(APL_GPIO_IRQ),
> > +	},
> > +	{
> > +		DEFINE_RES_MEM(APL_GPIO_SOUTHWEST_OFFSET,
> > APL_GPIO_SOUTHWEST_SIZE),
> > +		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]
> > = {
> > +	{
> > +		/* North */
> > +		.name = "apollolake-pinctrl",
> > +		.id = 0,
> > +		.num_resources = ARRAY_SIZE(apl_gpio_resources[0]),
> > +		.resources = apl_gpio_resources[0],
> > +		.ignore_resource_conflicts = true,
> > +	},
> > +	{
> > +		/* NorthWest */
> > +		.name = "apollolake-pinctrl",
> > +		.id = 1,
> > +		.num_resources = ARRAY_SIZE(apl_gpio_resources[1]),
> > +		.resources = apl_gpio_resources[1],
> > +		.ignore_resource_conflicts = true,
> > +	},
> > +	{
> > +		/* West */
> > +		.name = "apollolake-pinctrl",
> > +		.id = 2,
> > +		.num_resources = ARRAY_SIZE(apl_gpio_resources[2]),
> > +		.resources = apl_gpio_resources[2],
> > +		.ignore_resource_conflicts = true,
> > +	},
> > +	{
> > +		/* SouthWest */
> > +		.name = "apollolake-pinctrl",
> > +		.id = 3,
> > +		.num_resources = ARRAY_SIZE(apl_gpio_resources[3]),
> > +		.resources = apl_gpio_resources[3],
> > +		.ignore_resource_conflicts = true,
> > +	},
> > +};
> >  
> >  static struct mfd_cell lpc_ich_spi_cell = {
> >  	.name = "intel-spi",
> > @@ -1082,6 +1151,29 @@ 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;
> > +
> > +	ret = pci_p2sb_bar(dev, PCI_DEVFN(13, 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)
> >  {
> > @@ -1198,6 +1290,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)  
> 


^ permalink raw reply	[flat|nested] 54+ messages in thread

* Re: [PATCH v1 6/7] mfd: lpc_ich: Add support for pinctrl in non-ACPI system
  2021-04-12 16:01   ` Henning Schild
  2021-04-12 16:40     ` Henning Schild
@ 2021-04-12 16:51     ` Andy Shevchenko
  2021-04-12 17:27       ` Henning Schild
  1 sibling, 1 reply; 54+ messages in thread
From: Andy Shevchenko @ 2021-04-12 16:51 UTC (permalink / raw)
  To: Henning Schild
  Cc: Wolfram Sang, Jean Delvare, Lee Jones, Tan Jui Nee, Jim Quinlan,
	Jonathan Yong, Bjorn Helgaas, linux-kernel, linux-i2c, linux-pci,
	Jean Delvare, Peter Tyser, hdegoede

On Mon, Apr 12, 2021 at 06:01:06PM +0200, Henning Schild wrote:
> Am Mon, 8 Mar 2021 14:20:19 +0200
> schrieb Andy Shevchenko <andriy.shevchenko@linux.intel.com>:
> 
> > From: Tan Jui Nee <jui.nee.tan@intel.com>
> > 
> > Add support for non-ACPI systems, such as system that uses
> > Advanced Boot Loader (ABL) whereby a platform device has to be created
> > in order to bind with pin control and GPIO.
> > 
> > At the moment, Intel Apollo Lake In-Vehicle Infotainment (IVI) system
> > requires a driver to hide and unhide P2SB to lookup P2SB BAR and pass
> > the PCI BAR address to GPIO.

...

> > +/* Offset data for Apollo Lake GPIO controllers */
> > +#define APL_GPIO_SOUTHWEST_OFFSET	0xc00000
> > +#define APL_GPIO_SOUTHWEST_SIZE		0x654
> > +#define APL_GPIO_NORTHWEST_OFFSET	0xc40000
> > +#define APL_GPIO_NORTHWEST_SIZE		0x764
> > +#define APL_GPIO_NORTH_OFFSET		0xc50000
> > +#define APL_GPIO_NORTH_SIZE		0x76c
> 
> drivers/pinctrl/intel/pinctrl-broxton.c:653
> BXT_COMMUNITY(0, 77),
> 
> > +#define APL_GPIO_WEST_OFFSET		0xc70000
> > +#define APL_GPIO_WEST_SIZE		0x674
> 
> All these sizes correlate with 4 magic numbers from pinctrl-broxton.
> 
> SIZE - 0x500 (pad_base?) - 4 (no clue) / 8
> 
> It might be worth basing both numbers on a define and giving the magic
> numbers some names.

I didn't get this, sorry. The numbers above just precise sizes of the
resources. Actually they all one page anyway, so, I can drop magic of SIZEs and
leave only offsets.

> But all this seems like duplication of pinctrl-broxton, maybe the
> pinctrl driver should unhide the p2sb ...

Definitely should not. It's not a business of the pin control driver to know
how it has to be instantiated (or from what data). These offsets belong to the
platform description and since firmware hides the device without given an
appropriate ACPI device node, we have only one choice (assuming firmware is
carved in stone) -- board files.

P2SB on the other hand is a slice of many (independent) devices. There is no
"proper" place to unhide it except some core part of x86 / PCI.


-- 
With Best Regards,
Andy Shevchenko



^ permalink raw reply	[flat|nested] 54+ messages in thread

* Re: [PATCH v1 6/7] mfd: lpc_ich: Add support for pinctrl in non-ACPI system
  2021-04-12 16:40     ` Henning Schild
@ 2021-04-12 16:59       ` Andy Shevchenko
  2021-04-12 17:16         ` Henning Schild
  0 siblings, 1 reply; 54+ messages in thread
From: Andy Shevchenko @ 2021-04-12 16:59 UTC (permalink / raw)
  To: Henning Schild
  Cc: Wolfram Sang, Jean Delvare, Lee Jones, Tan Jui Nee, Jim Quinlan,
	Jonathan Yong, Bjorn Helgaas, linux-kernel, linux-i2c, linux-pci,
	Jean Delvare, Peter Tyser, hdegoede

On Mon, Apr 12, 2021 at 06:40:01PM +0200, Henning Schild wrote:
> Tan or Andy,
> 
> maybe you can point me to a user of that patch. I guess there might be
> an out-of-tree driver or userland code on how to use the GPIOs from
> there.

I'm confused. User of this patch is pinctrl-broxton driver.
It's in upstream.

Using GPIOs from it is something as done in a few drivers already
(Assuming we have no resources described in the ACPI). I.e. you need to
register in board file the GPIO mapping table with help of
devm_acpi_dev_add_driver_gpios() and use one of gpiod_get() family of functions
to request it.

In case of LEDs you simple describe GPIO device name in lookup table and
that's it. The drivers/platform/x86/pcengines-apuv2.c not the best but
will give you an idea how to use "leds-gpio" driver in board files.


> Feel free to send directly to me in case it is not published anywhere
> and should not yet be on the list, i could just use it for inspiration.
> A driver will likely be GPL anyways.

-- 
With Best Regards,
Andy Shevchenko



^ permalink raw reply	[flat|nested] 54+ messages in thread

* Re: [PATCH v1 6/7] mfd: lpc_ich: Add support for pinctrl in non-ACPI system
  2021-04-12 16:59       ` Andy Shevchenko
@ 2021-04-12 17:16         ` Henning Schild
  2021-04-12 17:34           ` Andy Shevchenko
  0 siblings, 1 reply; 54+ messages in thread
From: Henning Schild @ 2021-04-12 17:16 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: Wolfram Sang, Jean Delvare, Lee Jones, Tan Jui Nee, Jim Quinlan,
	Jonathan Yong, Bjorn Helgaas, linux-kernel, linux-i2c, linux-pci,
	Jean Delvare, Peter Tyser, hdegoede

Am Mon, 12 Apr 2021 19:59:05 +0300
schrieb Andy Shevchenko <andriy.shevchenko@linux.intel.com>:

> On Mon, Apr 12, 2021 at 06:40:01PM +0200, Henning Schild wrote:
> > Tan or Andy,
> > 
> > maybe you can point me to a user of that patch. I guess there might
> > be an out-of-tree driver or userland code on how to use the GPIOs
> > from there.  
> 
> I'm confused. User of this patch is pinctrl-broxton driver.
> It's in upstream.

Should this appear in /sys/class/gpio as chip so that pins can be
exported?

That is what i tried and failed with.

> Using GPIOs from it is something as done in a few drivers already
> (Assuming we have no resources described in the ACPI). I.e. you need
> to register in board file the GPIO mapping table with help of
> devm_acpi_dev_add_driver_gpios() and use one of gpiod_get() family of
> functions to request it.
> 
> In case of LEDs you simple describe GPIO device name in lookup table
> and that's it. The drivers/platform/x86/pcengines-apuv2.c not the
> best but will give you an idea how to use "leds-gpio" driver in board
> files.

I am aware of that driver and had a look at it. In order to figure out
the arguments for the macros/functions i was hoping for userland gpio
"export", but maybe that does not work here ...
For now i will assume that it does not show up in sysfs and can maybe
still be used, and try to build on top.

regards,
Henning

> 
> > Feel free to send directly to me in case it is not published
> > anywhere and should not yet be on the list, i could just use it for
> > inspiration. A driver will likely be GPL anyways.  
> 


^ permalink raw reply	[flat|nested] 54+ messages in thread

* Re: [PATCH v1 6/7] mfd: lpc_ich: Add support for pinctrl in non-ACPI system
  2021-04-12 16:51     ` Andy Shevchenko
@ 2021-04-12 17:27       ` Henning Schild
  2021-04-12 17:41         ` Andy Shevchenko
  0 siblings, 1 reply; 54+ messages in thread
From: Henning Schild @ 2021-04-12 17:27 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: Wolfram Sang, Jean Delvare, Lee Jones, Tan Jui Nee, Jim Quinlan,
	Jonathan Yong, Bjorn Helgaas, linux-kernel, linux-i2c, linux-pci,
	Jean Delvare, Peter Tyser, hdegoede

Am Mon, 12 Apr 2021 19:51:42 +0300
schrieb Andy Shevchenko <andriy.shevchenko@linux.intel.com>:

> On Mon, Apr 12, 2021 at 06:01:06PM +0200, Henning Schild wrote:
> > Am Mon, 8 Mar 2021 14:20:19 +0200
> > schrieb Andy Shevchenko <andriy.shevchenko@linux.intel.com>:
> >   
> > > From: Tan Jui Nee <jui.nee.tan@intel.com>
> > > 
> > > Add support for non-ACPI systems, such as system that uses
> > > Advanced Boot Loader (ABL) whereby a platform device has to be
> > > created in order to bind with pin control and GPIO.
> > > 
> > > At the moment, Intel Apollo Lake In-Vehicle Infotainment (IVI)
> > > system requires a driver to hide and unhide P2SB to lookup P2SB
> > > BAR and pass the PCI BAR address to GPIO.  
> 
> ...
> 
> > > +/* Offset data for Apollo Lake GPIO controllers */
> > > +#define APL_GPIO_SOUTHWEST_OFFSET	0xc00000
> > > +#define APL_GPIO_SOUTHWEST_SIZE		0x654
> > > +#define APL_GPIO_NORTHWEST_OFFSET	0xc40000
> > > +#define APL_GPIO_NORTHWEST_SIZE		0x764
> > > +#define APL_GPIO_NORTH_OFFSET		0xc50000
> > > +#define APL_GPIO_NORTH_SIZE		0x76c  
> > 
> > drivers/pinctrl/intel/pinctrl-broxton.c:653
> > BXT_COMMUNITY(0, 77),
> >   
> > > +#define APL_GPIO_WEST_OFFSET		0xc70000
> > > +#define APL_GPIO_WEST_SIZE		0x674  
> > 
> > All these sizes correlate with 4 magic numbers from pinctrl-broxton.
> > 
> > SIZE - 0x500 (pad_base?) - 4 (no clue) / 8
> > 
> > It might be worth basing both numbers on a define and giving the
> > magic numbers some names.  
> 
> I didn't get this, sorry. The numbers above just precise sizes of the
> resources. Actually they all one page anyway, so, I can drop magic of
> SIZEs and leave only offsets.

That precise size is also in the broxton driver, i think. Say we did
have

#define BXT_NORTH_COUNT 77
#define PAD_BASE 0x500

in some central place

then we could use

size = 0x500 + 8 * BXT_NORTH_COUNT + 4 (no clue what that is)

the same pattern would work for all those sizes and their
BXT_COMMUNITY(0, XX) counterparts

So the real size seems to be a function of the magic numbers in
BXT_COMMUNITY(0, XX)

Or simply take one page as you say.

> > But all this seems like duplication of pinctrl-broxton, maybe the
> > pinctrl driver should unhide the p2sb ...  
> 
> Definitely should not. It's not a business of the pin control driver
> to know how it has to be instantiated (or from what data). These
> offsets belong to the platform description and since firmware hides
> the device without given an appropriate ACPI device node, we have
> only one choice (assuming firmware is carved in stone) -- board files.
> 
> P2SB on the other hand is a slice of many (independent) devices.
> There is no "proper" place to unhide it except some core part of x86
> / PCI.

Got it, still the fact that there are 4 regions/communities is also part
of the broxton driver so there is duplication.

regards,
Henning


^ permalink raw reply	[flat|nested] 54+ messages in thread

* Re: [PATCH v1 6/7] mfd: lpc_ich: Add support for pinctrl in non-ACPI system
  2021-04-12 17:16         ` Henning Schild
@ 2021-04-12 17:34           ` Andy Shevchenko
  2021-04-13  6:47             ` Henning Schild
  0 siblings, 1 reply; 54+ messages in thread
From: Andy Shevchenko @ 2021-04-12 17:34 UTC (permalink / raw)
  To: Henning Schild
  Cc: Wolfram Sang, Jean Delvare, Lee Jones, Tan Jui Nee, Jim Quinlan,
	Jonathan Yong, Bjorn Helgaas, linux-kernel, linux-i2c, linux-pci,
	Jean Delvare, Peter Tyser, hdegoede

On Mon, Apr 12, 2021 at 07:16:53PM +0200, Henning Schild wrote:
> Am Mon, 12 Apr 2021 19:59:05 +0300
> schrieb Andy Shevchenko <andriy.shevchenko@linux.intel.com>:
> > On Mon, Apr 12, 2021 at 06:40:01PM +0200, Henning Schild wrote:
> > > Tan or Andy,
> > > 
> > > maybe you can point me to a user of that patch. I guess there might
> > > be an out-of-tree driver or userland code on how to use the GPIOs
> > > from there.  
> > 
> > I'm confused. User of this patch is pinctrl-broxton driver.
> > It's in upstream.
> 
> Should this appear in /sys/class/gpio as chip so that pins can be
> exported?

No. Sysfs interface is deprecated. It should appear as /dev/gpiochip0 or so.

> That is what i tried and failed with.
> 
> > Using GPIOs from it is something as done in a few drivers already
> > (Assuming we have no resources described in the ACPI). I.e. you need
> > to register in board file the GPIO mapping table with help of
> > devm_acpi_dev_add_driver_gpios() and use one of gpiod_get() family of
> > functions to request it.
> > 
> > In case of LEDs you simple describe GPIO device name in lookup table
> > and that's it. The drivers/platform/x86/pcengines-apuv2.c not the
> > best but will give you an idea how to use "leds-gpio" driver in board
> > files.
> 
> I am aware of that driver and had a look at it. In order to figure out
> the arguments for the macros/functions i was hoping for userland gpio
> "export", but maybe that does not work here ...
> For now i will assume that it does not show up in sysfs and can maybe
> still be used, and try to build on top.

Just switch to use libgpiod and associated tools / bindings in user space.
Sysfs ABI is not being developed anymore.

-- 
With Best Regards,
Andy Shevchenko



^ permalink raw reply	[flat|nested] 54+ messages in thread

* Re: [PATCH v1 6/7] mfd: lpc_ich: Add support for pinctrl in non-ACPI system
  2021-04-12 17:27       ` Henning Schild
@ 2021-04-12 17:41         ` Andy Shevchenko
  0 siblings, 0 replies; 54+ messages in thread
From: Andy Shevchenko @ 2021-04-12 17:41 UTC (permalink / raw)
  To: Henning Schild
  Cc: Wolfram Sang, Jean Delvare, Lee Jones, Tan Jui Nee, Jim Quinlan,
	Jonathan Yong, Bjorn Helgaas, linux-kernel, linux-i2c, linux-pci,
	Jean Delvare, Peter Tyser, hdegoede

On Mon, Apr 12, 2021 at 07:27:14PM +0200, Henning Schild wrote:
> Am Mon, 12 Apr 2021 19:51:42 +0300
> schrieb Andy Shevchenko <andriy.shevchenko@linux.intel.com>:
> > On Mon, Apr 12, 2021 at 06:01:06PM +0200, Henning Schild wrote:
> > > Am Mon, 8 Mar 2021 14:20:19 +0200
> > > schrieb Andy Shevchenko <andriy.shevchenko@linux.intel.com>:

...

> > > > +#define APL_GPIO_NORTH_OFFSET		0xc50000
> > > > +#define APL_GPIO_NORTH_SIZE		0x76c  
> > > 
> > > drivers/pinctrl/intel/pinctrl-broxton.c:653
> > > BXT_COMMUNITY(0, 77),
> > >   
> > > > +#define APL_GPIO_WEST_OFFSET		0xc70000
> > > > +#define APL_GPIO_WEST_SIZE		0x674  
> > > 
> > > All these sizes correlate with 4 magic numbers from pinctrl-broxton.
> > > 
> > > SIZE - 0x500 (pad_base?) - 4 (no clue) / 8
> > > 
> > > It might be worth basing both numbers on a define and giving the
> > > magic numbers some names.  
> > 
> > I didn't get this, sorry. The numbers above just precise sizes of the
> > resources. Actually they all one page anyway, so, I can drop magic of
> > SIZEs and leave only offsets.
> 
> That precise size is also in the broxton driver, i think. Say we did
> have
> 
> #define BXT_NORTH_COUNT 77
> #define PAD_BASE 0x500
> 
> in some central place
> 
> then we could use
> 
> size = 0x500 + 8 * BXT_NORTH_COUNT + 4 (no clue what that is)
> 
> the same pattern would work for all those sizes and their
> BXT_COMMUNITY(0, XX) counterparts
> 
> So the real size seems to be a function of the magic numbers in
> BXT_COMMUNITY(0, XX)
> 
> Or simply take one page as you say.

No, not this way. We are really trying hard *not* to put *that* magic into
the code. Just FYI that SIZEs I have calculated myself, but these SIZEs
are *not* the same as the ones used in pinctrl-broxton *semantically*.

One if for resource provider, one is for consumer. They are simply different
in this sense.

> > > But all this seems like duplication of pinctrl-broxton, maybe the
> > > pinctrl driver should unhide the p2sb ...  
> > 
> > Definitely should not. It's not a business of the pin control driver
> > to know how it has to be instantiated (or from what data). These
> > offsets belong to the platform description and since firmware hides
> > the device without given an appropriate ACPI device node, we have
> > only one choice (assuming firmware is carved in stone) -- board files.
> > 
> > P2SB on the other hand is a slice of many (independent) devices.
> > There is no "proper" place to unhide it except some core part of x86
> > / PCI.
> 
> Got it, still the fact that there are 4 regions/communities is also part
> of the broxton driver so there is duplication.

See above. I guess here is a misunderstanding behind meaning of the (same)
numbers in different parts. Technically we may unify them, but it will be
a layering violation.

-- 
With Best Regards,
Andy Shevchenko



^ permalink raw reply	[flat|nested] 54+ messages in thread

* Re: [PATCH v1 6/7] mfd: lpc_ich: Add support for pinctrl in non-ACPI system
  2021-04-12 17:34           ` Andy Shevchenko
@ 2021-04-13  6:47             ` Henning Schild
  0 siblings, 0 replies; 54+ messages in thread
From: Henning Schild @ 2021-04-13  6:47 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: Wolfram Sang, Jean Delvare, Lee Jones, Tan Jui Nee, Jim Quinlan,
	Jonathan Yong, Bjorn Helgaas, linux-kernel, linux-i2c, linux-pci,
	Jean Delvare, Peter Tyser, hdegoede

Am Mon, 12 Apr 2021 20:34:45 +0300
schrieb Andy Shevchenko <andriy.shevchenko@linux.intel.com>:

> On Mon, Apr 12, 2021 at 07:16:53PM +0200, Henning Schild wrote:
> > Am Mon, 12 Apr 2021 19:59:05 +0300
> > schrieb Andy Shevchenko <andriy.shevchenko@linux.intel.com>:  
> > > On Mon, Apr 12, 2021 at 06:40:01PM +0200, Henning Schild wrote:  
> > > > Tan or Andy,
> > > > 
> > > > maybe you can point me to a user of that patch. I guess there
> > > > might be an out-of-tree driver or userland code on how to use
> > > > the GPIOs from there.    
> > > 
> > > I'm confused. User of this patch is pinctrl-broxton driver.
> > > It's in upstream.  
> > 
> > Should this appear in /sys/class/gpio as chip so that pins can be
> > exported?  
> 
> No. Sysfs interface is deprecated. It should appear as /dev/gpiochip0
> or so.

Ok, just found that there is a null pointer deref in the probe function
of the pinctrl driver, looking into that.

Meanwhile i think i will need a similar patch for
pinctrl-sunrisepoint.c for that wdt, do you happen to have that as
well? Or a spec where to find all the magic numbers.

regards,
Henning

> 
> > That is what i tried and failed with.
> >   
> > > Using GPIOs from it is something as done in a few drivers already
> > > (Assuming we have no resources described in the ACPI). I.e. you
> > > need to register in board file the GPIO mapping table with help of
> > > devm_acpi_dev_add_driver_gpios() and use one of gpiod_get()
> > > family of functions to request it.
> > > 
> > > In case of LEDs you simple describe GPIO device name in lookup
> > > table and that's it. The drivers/platform/x86/pcengines-apuv2.c
> > > not the best but will give you an idea how to use "leds-gpio"
> > > driver in board files.  
> > 
> > I am aware of that driver and had a look at it. In order to figure
> > out the arguments for the macros/functions i was hoping for
> > userland gpio "export", but maybe that does not work here ...
> > For now i will assume that it does not show up in sysfs and can
> > maybe still be used, and try to build on top.  
> 
> Just switch to use libgpiod and associated tools / bindings in user
> space. Sysfs ABI is not being developed anymore.
> 


^ permalink raw reply	[flat|nested] 54+ messages in thread

* Re: [rfc, PATCH v1 0/7] PCI: introduce p2sb helper
  2021-03-08 12:20 [rfc, PATCH v1 0/7] PCI: introduce p2sb helper Andy Shevchenko
                   ` (7 preceding siblings ...)
  2021-03-13  9:25 ` [rfc, PATCH v1 0/7] PCI: introduce p2sb helper Henning Schild
@ 2021-06-10  9:02 ` Henning Schild
  2021-06-10 10:14   ` Andy Shevchenko
  2021-11-26 15:43 ` Andy Shevchenko
  9 siblings, 1 reply; 54+ messages in thread
From: Henning Schild @ 2021-06-10  9:02 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: Wolfram Sang, Jean Delvare, Lee Jones, Tan Jui Nee, Jim Quinlan,
	Jonathan Yong, Bjorn Helgaas, linux-kernel, linux-i2c, linux-pci,
	Jean Delvare, Peter Tyser, hdegoede

Am Mon, 8 Mar 2021 14:20:13 +0200
schrieb Andy Shevchenko <andriy.shevchenko@linux.intel.com>:

> There are a few users and even at least one more is coming
> that would like to utilize p2sb mechanisms like hide/unhide
> a device from 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.

That bit is especially interesting. Making pinctl*lake initialize when
ACPI IDs are missing and p2sb is hidden.

However i have seen pinctl-broxton get confused because it was trying
to come up twice on a system that has the ACPI entries. Once as
"INT3452" and second as "apollolake-pinctrl". They should probably
mutually exclude each other. And the two different names for "the
same"? thing make it impossible to write a driver using those GPIOs.
Unless it would try and look up both variants or not looking up with
gpiochip.label.

I would also need that "enable GPIO w/o ACPI" for skylake. I think it
would be generally useful if the GPIO controllers would be enabled not
depending on ACPI, and coming up with only one "label" to build on top.

regards,
Henning

> Please, comment on the approach and individual patches.
> 
> (Since it's cross subsystem, the PCI seems like a main one and
>  I think it makes sense to route it thru it with immutable tag
>  or branch provided for the others).
> 
> Andy Shevchenko (5):
>   PCI: Introduce pci_bus_*() printing macros when device is not
>     available
>   PCI: Convert __pci_read_base() to __pci_bus_read_base()
>   mfd: lpc_ich: Factor out lpc_ich_enable_spi_write()
>   mfd: lpc_ich: Switch to generic pci_p2sb_bar()
>   i2c: i801: convert to use common P2SB accessor
> 
> Jonathan Yong (1):
>   PCI: New Primary to Sideband (P2SB) bridge support library
> 
> 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 |  40 +++-------
>  drivers/mfd/Kconfig           |   1 +
>  drivers/mfd/lpc_ich.c         | 135
> +++++++++++++++++++++++++++++----- drivers/pci/Kconfig           |
> 8 ++ drivers/pci/Makefile          |   1 +
>  drivers/pci/pci-p2sb.c        |  89 ++++++++++++++++++++++
>  drivers/pci/pci.h             |  13 +++-
>  drivers/pci/probe.c           |  81 ++++++++++----------
>  include/linux/pci-p2sb.h      |  28 +++++++
>  include/linux/pci.h           |   9 +++
>  11 files changed, 313 insertions(+), 93 deletions(-)
>  create mode 100644 drivers/pci/pci-p2sb.c
>  create mode 100644 include/linux/pci-p2sb.h
> 


^ permalink raw reply	[flat|nested] 54+ messages in thread

* Re: [rfc, PATCH v1 0/7] PCI: introduce p2sb helper
  2021-06-10  9:02 ` Henning Schild
@ 2021-06-10 10:14   ` Andy Shevchenko
  2021-06-10 13:48     ` Henning Schild
  0 siblings, 1 reply; 54+ messages in thread
From: Andy Shevchenko @ 2021-06-10 10:14 UTC (permalink / raw)
  To: Henning Schild
  Cc: Andy Shevchenko, Wolfram Sang, Jean Delvare, Lee Jones,
	Tan Jui Nee, Jim Quinlan, Jonathan Yong, Bjorn Helgaas,
	Linux Kernel Mailing List, linux-i2c, linux-pci, Jean Delvare,
	Peter Tyser, Hans de Goede

On Thu, Jun 10, 2021 at 12:14 PM Henning Schild
<henning.schild@siemens.com> wrote:
>
> Am Mon, 8 Mar 2021 14:20:13 +0200
> schrieb Andy Shevchenko <andriy.shevchenko@linux.intel.com>:
>
> > There are a few users and even at least one more is coming
> > that would like to utilize p2sb mechanisms like hide/unhide
> > a device from 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.
>
> That bit is especially interesting. Making pinctl*lake initialize when
> ACPI IDs are missing and p2sb is hidden.
>
> However i have seen pinctl-broxton get confused because it was trying
> to come up twice on a system that has the ACPI entries. Once as
> "INT3452" and second as "apollolake-pinctrl". They should probably
> mutually exclude each other. And the two different names for "the
> same"? thing make it impossible to write a driver using those GPIOs.

Then it's clearly told that BIOS provides confusing data, it exposes
the ACPI device and hides it in p2sb, how is it even supposed to work?

I consider only these are valid:
 - ACPI device is provided and it's enabled (status = 15) => work with
ACPI enumeration
 - no ACPI device provided and it's hidden or not by p2sb => work via board file
 - no ACPI device provided and no device needed / present => no driver is needed

> Unless it would try and look up both variants or not looking up with
> gpiochip.label.
>
> I would also need that "enable GPIO w/o ACPI" for skylake.

Not a problem to add a platform driver name there or actually for all
of the Intel pin control drivers (depends what suits better to the
current design).

>  I think it
> would be generally useful if the GPIO controllers would be enabled not
> depending on ACPI, and coming up with only one "label" to build on top.

I didn't get what 'label' means here...

> > Please, comment on the approach and individual patches.
> >
> > (Since it's cross subsystem, the PCI seems like a main one and
> >  I think it makes sense to route it thru it with immutable tag
> >  or branch provided for the others).


-- 
With Best Regards,
Andy Shevchenko

^ permalink raw reply	[flat|nested] 54+ messages in thread

* Re: [rfc, PATCH v1 0/7] PCI: introduce p2sb helper
  2021-06-10 10:14   ` Andy Shevchenko
@ 2021-06-10 13:48     ` Henning Schild
  2021-06-10 14:04       ` Andy Shevchenko
  0 siblings, 1 reply; 54+ messages in thread
From: Henning Schild @ 2021-06-10 13:48 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: Andy Shevchenko, Wolfram Sang, Jean Delvare, Lee Jones,
	Tan Jui Nee, Jim Quinlan, Jonathan Yong, Bjorn Helgaas,
	Linux Kernel Mailing List, linux-i2c, linux-pci, Jean Delvare,
	Peter Tyser, Hans de Goede

Am Thu, 10 Jun 2021 13:14:49 +0300
schrieb Andy Shevchenko <andy.shevchenko@gmail.com>:

> On Thu, Jun 10, 2021 at 12:14 PM Henning Schild
> <henning.schild@siemens.com> wrote:
> >
> > Am Mon, 8 Mar 2021 14:20:13 +0200
> > schrieb Andy Shevchenko <andriy.shevchenko@linux.intel.com>:
> >  
> > > There are a few users and even at least one more is coming
> > > that would like to utilize p2sb mechanisms like hide/unhide
> > > a device from 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.  
> >
> > That bit is especially interesting. Making pinctl*lake initialize
> > when ACPI IDs are missing and p2sb is hidden.
> >
> > However i have seen pinctl-broxton get confused because it was
> > trying to come up twice on a system that has the ACPI entries. Once
> > as "INT3452" and second as "apollolake-pinctrl". They should
> > probably mutually exclude each other. And the two different names
> > for "the same"? thing make it impossible to write a driver using
> > those GPIOs.  
> 
> Then it's clearly told that BIOS provides confusing data, it exposes
> the ACPI device and hides it in p2sb, how is it even supposed to work?

The patchset works fine on a machine with hidden p2sb and no ACPI,
except for the NULL pointer issue i sent that patch for.

The problem appeared with the patchset being used on a machine having
ACPI entries and a visible p2sb. 

> I consider only these are valid:
>  - ACPI device is provided and it's enabled (status = 15) => work with
> ACPI enumeration
>  - no ACPI device provided and it's hidden or not by p2sb => work via
> board file
>  - no ACPI device provided and no device needed / present => no
> driver is needed
> 
> > Unless it would try and look up both variants or not looking up with
> > gpiochip.label.
> >
> > I would also need that "enable GPIO w/o ACPI" for skylake.  
> 
> Not a problem to add a platform driver name there or actually for all
> of the Intel pin control drivers (depends what suits better to the
> current design).
> 
> >  I think it
> > would be generally useful if the GPIO controllers would be enabled
> > not depending on ACPI, and coming up with only one "label" to build
> > on top.  
> 
> I didn't get what 'label' means here...

The name of the gpiochip /sys/class/gpiochipxxx/label or the first arg
to GPIO_LOOKUP_IDX
It seems to me that the very same device driver can come up as
"apollolake-pinctrl.0" or "INT3452.0" depending on ACPI table entries.

Henning

> > > Please, comment on the approach and individual patches.
> > >
> > > (Since it's cross subsystem, the PCI seems like a main one and
> > >  I think it makes sense to route it thru it with immutable tag
> > >  or branch provided for the others).  
> 
> 


^ permalink raw reply	[flat|nested] 54+ messages in thread

* Re: [rfc, PATCH v1 0/7] PCI: introduce p2sb helper
  2021-06-10 13:48     ` Henning Schild
@ 2021-06-10 14:04       ` Andy Shevchenko
  0 siblings, 0 replies; 54+ messages in thread
From: Andy Shevchenko @ 2021-06-10 14:04 UTC (permalink / raw)
  To: Henning Schild
  Cc: Andy Shevchenko, Wolfram Sang, Jean Delvare, Lee Jones,
	Tan Jui Nee, Jim Quinlan, Jonathan Yong, Bjorn Helgaas,
	Linux Kernel Mailing List, linux-i2c, linux-pci, Jean Delvare,
	Peter Tyser, Hans de Goede

On Thu, Jun 10, 2021 at 4:48 PM Henning Schild
<henning.schild@siemens.com> wrote:
>
> Am Thu, 10 Jun 2021 13:14:49 +0300
> schrieb Andy Shevchenko <andy.shevchenko@gmail.com>:
>
> > On Thu, Jun 10, 2021 at 12:14 PM Henning Schild
> > <henning.schild@siemens.com> wrote:
> > >
> > > Am Mon, 8 Mar 2021 14:20:13 +0200
> > > schrieb Andy Shevchenko <andriy.shevchenko@linux.intel.com>:
> > >
> > > > There are a few users and even at least one more is coming
> > > > that would like to utilize p2sb mechanisms like hide/unhide
> > > > a device from 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.
> > >
> > > That bit is especially interesting. Making pinctl*lake initialize
> > > when ACPI IDs are missing and p2sb is hidden.
> > >
> > > However i have seen pinctl-broxton get confused because it was
> > > trying to come up twice on a system that has the ACPI entries. Once
> > > as "INT3452" and second as "apollolake-pinctrl". They should
> > > probably mutually exclude each other. And the two different names
> > > for "the same"? thing make it impossible to write a driver using
> > > those GPIOs.
> >
> > Then it's clearly told that BIOS provides confusing data, it exposes
> > the ACPI device and hides it in p2sb, how is it even supposed to work?
>
> The patchset works fine on a machine with hidden p2sb and no ACPI,
> except for the NULL pointer issue i sent that patch for.
>
> The problem appeared with the patchset being used on a machine having
> ACPI entries and a visible p2sb.

Yep, got it. So, basically we have to do something like call
acpi_dev_present() and forbid the platform device enumeration in this
case.

> > I consider only these are valid:
> >  - ACPI device is provided and it's enabled (status = 15) => work with
> > ACPI enumeration
> >  - no ACPI device provided and it's hidden or not by p2sb => work via
> > board file
> >  - no ACPI device provided and no device needed / present => no
> > driver is needed
> >
> > > Unless it would try and look up both variants or not looking up with
> > > gpiochip.label.
> > >
> > > I would also need that "enable GPIO w/o ACPI" for skylake.
> >
> > Not a problem to add a platform driver name there or actually for all
> > of the Intel pin control drivers (depends what suits better to the
> > current design).
> >
> > >  I think it
> > > would be generally useful if the GPIO controllers would be enabled
> > > not depending on ACPI, and coming up with only one "label" to build
> > > on top.
> >
> > I didn't get what 'label' means here...
>
> The name of the gpiochip /sys/class/gpiochipxxx/label or the first arg
> to GPIO_LOOKUP_IDX
> It seems to me that the very same device driver can come up as
> "apollolake-pinctrl.0" or "INT3452.0" depending on ACPI table entries.

Which is not a problem. Or is it? The proper way is to use character
devices and find the controller based on other means than the device
instance name, but user space also can cope with these two, Since we
never had a platform that did it in the upstream there is no formal
ABI breakage or so.

> > > > Please, comment on the approach and individual patches.
> > > >
> > > > (Since it's cross subsystem, the PCI seems like a main one and
> > > >  I think it makes sense to route it thru it with immutable tag
> > > >  or branch provided for the others).


-- 
With Best Regards,
Andy Shevchenko

^ permalink raw reply	[flat|nested] 54+ messages in thread

* Re: [PATCH v1 3/7] PCI: New Primary to Sideband (P2SB) bridge support library
  2021-04-06 13:40             ` Henning Schild
@ 2021-07-12 12:11               ` Andy Shevchenko
  0 siblings, 0 replies; 54+ messages in thread
From: Andy Shevchenko @ 2021-07-12 12:11 UTC (permalink / raw)
  To: Henning Schild
  Cc: Enrico Weigelt, metux IT consult, Bjorn Helgaas, Wolfram Sang,
	Jean Delvare, Lee Jones, Tan Jui Nee, Jim Quinlan, Jonathan Yong,
	Bjorn Helgaas, linux-kernel, linux-i2c, linux-pci, Jean Delvare,
	Peter Tyser, hdegoede

On Tue, Apr 06, 2021 at 03:40:01PM +0200, Henning Schild wrote:
> Am Fri, 2 Apr 2021 15:09:12 +0200
> schrieb "Enrico Weigelt, metux IT consult" <lkml@metux.net>:
> 
> > On 09.03.21 09:42, Henning Schild wrote:
> > 
> > > The device will respond to MMIO while being hidden. I am afraid
> > > nothing stops a collision, except for the assumption that the BIOS
> > > is always right and PCI devices never get remapped. But just
> > > guessing here.  
> > 
> > What could go wrong if it is remapped, except that this driver would
> > write to the wrong mmio space ?
> > 
> > If it's unhidden, pci-core should see it and start the usual probing,
> > right ?
> 
> I have seen this guy exposed to Linux on coreboot machines. No issues.
> But i can imagine BIOSs that somehow make use of the device and assume
> it wont move. So we would at least need a parameter to allow keeping
> that device hidden, or "fixed" in memory.

I'm wondering if they have pin control device described in the ACPI.
If so, how in that case you prevent double initialisation?

We would need to check both: P2SB and ACPI tables. Basically if we enable P2SB
as a PCI device we may create a corresponding driver (somewhere under
drivers/pci or PDx86) and check in its probe that ACPI device is also present
and functional.

-- 
With Best Regards,
Andy Shevchenko



^ permalink raw reply	[flat|nested] 54+ messages in thread

* Re: [PATCH v1 3/7] PCI: New Primary to Sideband (P2SB) bridge support library
  2021-04-01 18:32           ` Mika Westerberg
@ 2021-07-12 12:13             ` Andy Shevchenko
  0 siblings, 0 replies; 54+ messages in thread
From: Andy Shevchenko @ 2021-07-12 12:13 UTC (permalink / raw)
  To: Mika Westerberg
  Cc: Henning Schild, Wolfram Sang, Jean Delvare, Lee Jones,
	Tan Jui Nee, Jim Quinlan, Jonathan Yong, Bjorn Helgaas,
	linux-kernel, linux-i2c, linux-pci, Jean Delvare, Peter Tyser,
	hdegoede

On Thu, Apr 01, 2021 at 09:32:36PM +0300, Mika Westerberg wrote:
> On Thu, Apr 01, 2021 at 09:22:02PM +0300, Andy Shevchenko wrote:
> > On Thu, Apr 01, 2021 at 09:06:17PM +0300, Mika Westerberg wrote:
> > > On Thu, Apr 01, 2021 at 06:43:11PM +0300, Andy Shevchenko wrote:
> > > > On Sat, Mar 13, 2021 at 10:45:57AM +0100, Henning Schild wrote:
> > > > > Am Mon, 8 Mar 2021 14:20:16 +0200
> > > > > schrieb Andy Shevchenko <andriy.shevchenko@linux.intel.com>:
> > > > 
> > > > ...
> > > > 
> > > > > > + * pci_p2sb_bar - Get Primary to Sideband bridge (P2SB) device BAR
> > > > > > + * @pdev:	PCI device to get a PCI bus to communicate with
> > > > > > + * @devfn:	PCI slot and function to communicate with
> > > > > > + * @mem:	memory resource to be filled in
> > > > > 
> > > > > Do we really need that many arguments to it?
> > > > > 
> > > > > Before i had, in a platform driver that never had its own pci_dev or bus
> > > > > 
> > > > >   res->start = simatic_ipc_get_membase0(PCI_DEVFN(13, 0));
> > > > >   if (res-start == 0)
> > > > >     return -ENODEV;
> > > > > 
> > > > > So helper only asked for the devfn, returned base and no dedicated
> > > > > error code.
> > > > > 
> > > > > With this i need
> > > > > 
> > > > >   struct pci_bus *bus = pci_find_bus(0, 0);
> > > > >   struct pci_dev *pci_dev = bus->self;
> > > > >   unsigned int magic_i_do_not_want =  PCI_DEVFN(13, 0);
> > > > 
> > > > What confuses me is the use for SPI NOR controller on Broxton. And I think
> > > > we actually can indeed hide all this under the hood by exposing P2SB to the OS.
> > > > 
> > > > Mika, what do you think?
> > > 
> > > Not sure I follow. Do you mean we force unhide P2SB and then bind (MFD)
> > > driver to that?
> > 
> > Not MFD, SPI NOR (if I understood correctly the code in MFD driver for SPI NOR
> > in regards to P2SB case).
> 
> I mean a new MFD driver that binds to the P2SB and that one then exposes
> the stuff needed by the SPI-NOR driver.

But as far as I understood it doesn't binds to P2SB since we do not have that
device present at PCI enumeration stage.

Maybe at the end of the day the P2SB driver should be located in drivers/mfd
and take over the SPI NOR enumeration as well on platforms in question?

-- 
With Best Regards,
Andy Shevchenko



^ permalink raw reply	[flat|nested] 54+ messages in thread

* Re: [PATCH v1 3/7] PCI: New Primary to Sideband (P2SB) bridge support library
  2021-04-01 18:44                 ` Bjorn Helgaas
@ 2021-07-12 12:15                   ` Andy Shevchenko
  2021-11-26 15:10                     ` Andy Shevchenko
  0 siblings, 1 reply; 54+ messages in thread
From: Andy Shevchenko @ 2021-07-12 12:15 UTC (permalink / raw)
  To: Bjorn Helgaas
  Cc: Henning Schild, Wolfram Sang, Jean Delvare, Lee Jones,
	Tan Jui Nee, Jim Quinlan, Jonathan Yong, Bjorn Helgaas,
	linux-kernel, linux-i2c, linux-pci, Jean Delvare, Peter Tyser,
	hdegoede

On Thu, Apr 01, 2021 at 01:44:46PM -0500, Bjorn Helgaas wrote:
> On Thu, Apr 01, 2021 at 09:23:04PM +0300, Andy Shevchenko wrote:
> > On Thu, Apr 01, 2021 at 11:42:56AM -0500, Bjorn Helgaas wrote:
> > > On Thu, Apr 01, 2021 at 06:45:02PM +0300, Andy Shevchenko wrote:
> > > > On Tue, Mar 09, 2021 at 09:42:52AM +0100, Henning Schild wrote:
> > > > > Am Mon, 8 Mar 2021 19:42:21 -0600
> > > > > schrieb Bjorn Helgaas <helgaas@kernel.org>:
> > > > > > On Mon, Mar 08, 2021 at 09:16:50PM +0200, Andy Shevchenko wrote:
> > > > > > > On Mon, Mar 08, 2021 at 12:52:12PM -0600, Bjorn Helgaas wrote:  
> > > > > > > > On Mon, Mar 08, 2021 at 02:20:16PM +0200, Andy Shevchenko wrote:  
> > > > 
> > > > ...
> > > > 
> > > > > > > > > +	/* Read the first BAR of the device in question */
> > > > > > > > > +	__pci_bus_read_base(bus, devfn, pci_bar_unknown, mem,
> > > > > > > > > PCI_BASE_ADDRESS_0, true);  
> > > > > > > > 
> > > > > > > > I don't get this.  Apparently this normally hidden device is
> > > > > > > > consuming PCI address space.  The PCI core needs to know
> > > > > > > > about this.  If it doesn't, the PCI core may assign this
> > > > > > > > space to another device.  
> > > > > > > 
> > > > > > > Right, it returns all 1:s to any request so PCI core *thinks*
> > > > > > > it's plugged off (like D3cold or so).  
> > > > > > 
> > > > > > I'm asking about the MMIO address space.  The BAR is a register
> > > > > > in config space.  AFAICT, clearing P2SBC_HIDE_BYTE makes that
> > > > > > BAR visible.  The BAR describes a region of PCI address space.
> > > > > > It looks like setting P2SBC_HIDE_BIT makes the BAR disappear
> > > > > > from config space, but it sounds like the PCI address space
> > > > > > *described* by the BAR is still claimed by the device.  If the
> > > > > > device didn't respond to that MMIO space, you would have no
> > > > > > reason to read the BAR at all.
> > > > > > 
> > > > > > So what keeps the PCI core from assigning that MMIO space to
> > > > > > another device?
> > > > > 
> > > > > The device will respond to MMIO while being hidden. I am afraid
> > > > > nothing stops a collision, except for the assumption that the BIOS
> > > > > is always right and PCI devices never get remapped. But just
> > > > > guessing here.
> > > > > 
> > > > > I have seen devices with coreboot having the P2SB visible, and
> > > > > most likely relocatable. Making it visible in Linux and not hiding
> > > > > it again might work, but probably only as long as Linux will not
> > > > > relocate it.  Which i am afraid might seriously upset the BIOS,
> > > > > depending on what a device does with those GPIOs and which parts
> > > > > are implemented in the BIOS.
> > > > 
> > > > So the question is, do we have knobs in PCI core to mark device
> > > > fixes in terms of BARs, no relocation must be applied, no other
> > > > devices must have the region?
> > > 
> > > I think the closest thing is the IORESOURCE_PCI_FIXED bit that we use
> > > for things that must not be moved.  Generally PCI resources are
> > > associated with a pci_dev, and we set IORESOURCE_PCI_FIXED for BARs,
> > > e.g., dev->resource[n].  We do that for IDE legacy regions (see
> > > LEGACY_IO_RESOURCE), Langwell devices (pci_fixed_bar_fixup()),
> > > "enhanced allocation" (pci_ea_flags()), and some quirks (quirk_io()).
> > > 
> > > In your case, the device is hidden so it doesn't respond to config
> > > accesses, so there is no pci_dev for it.
> > 
> > Yes, and the idea is to unhide it on the early stage.
> > Would it be possible to quirk it to fix the IO resources?
> 
> If I read your current patch right, it unhides the device, reads the
> BAR, then hides the device again.  I didn't see that it would create a
> pci_dev for it.
> 
> If you unhide it and then enumerate it normally (and mark the BAR as
> IORESOURCE_PCI_FIXED to make sure we never move it), that might work.
> Then there should be a pci_dev for it, and it would then show up in
> sysfs, lspci, etc.  And we should insert the BAR in iomem_resource, so
> we should see it in /proc/iomem and we won't accidentally put
> something else on top of it.

If the PCI device is present and we have ACPI description for the one or more
devices (currently pin control), wouldn't be a conflicting resources issue?

When would be the suitable place to avoid that?

> > > resource, fills it in, sets IORESOURCE_PCI_FIXED, and does something
> > > similar to pci_claim_resource()?

-- 
With Best Regards,
Andy Shevchenko



^ permalink raw reply	[flat|nested] 54+ messages in thread

* Re: [PATCH v1 3/7] PCI: New Primary to Sideband (P2SB) bridge support library
  2021-07-12 12:15                   ` Andy Shevchenko
@ 2021-11-26 15:10                     ` Andy Shevchenko
  0 siblings, 0 replies; 54+ messages in thread
From: Andy Shevchenko @ 2021-11-26 15:10 UTC (permalink / raw)
  To: Bjorn Helgaas
  Cc: Henning Schild, Wolfram Sang, Jean Delvare, Lee Jones,
	Tan Jui Nee, Jim Quinlan, Jonathan Yong, Bjorn Helgaas,
	linux-kernel, linux-i2c, linux-pci, Jean Delvare, Peter Tyser,
	hdegoede

On Mon, Jul 12, 2021 at 03:15:17PM +0300, Andy Shevchenko wrote:
> On Thu, Apr 01, 2021 at 01:44:46PM -0500, Bjorn Helgaas wrote:
> > On Thu, Apr 01, 2021 at 09:23:04PM +0300, Andy Shevchenko wrote:
> > > On Thu, Apr 01, 2021 at 11:42:56AM -0500, Bjorn Helgaas wrote:
> > > > On Thu, Apr 01, 2021 at 06:45:02PM +0300, Andy Shevchenko wrote:
> > > > > On Tue, Mar 09, 2021 at 09:42:52AM +0100, Henning Schild wrote:
> > > > > > Am Mon, 8 Mar 2021 19:42:21 -0600
> > > > > > schrieb Bjorn Helgaas <helgaas@kernel.org>:
> > > > > > > On Mon, Mar 08, 2021 at 09:16:50PM +0200, Andy Shevchenko wrote:
> > > > > > > > On Mon, Mar 08, 2021 at 12:52:12PM -0600, Bjorn Helgaas wrote:  
> > > > > > > > > On Mon, Mar 08, 2021 at 02:20:16PM +0200, Andy Shevchenko wrote:  
> > > > > 
> > > > > ...
> > > > > 
> > > > > > > > > > +	/* Read the first BAR of the device in question */
> > > > > > > > > > +	__pci_bus_read_base(bus, devfn, pci_bar_unknown, mem,
> > > > > > > > > > PCI_BASE_ADDRESS_0, true);  
> > > > > > > > > 
> > > > > > > > > I don't get this.  Apparently this normally hidden device is
> > > > > > > > > consuming PCI address space.  The PCI core needs to know
> > > > > > > > > about this.  If it doesn't, the PCI core may assign this
> > > > > > > > > space to another device.  
> > > > > > > > 
> > > > > > > > Right, it returns all 1:s to any request so PCI core *thinks*
> > > > > > > > it's plugged off (like D3cold or so).  
> > > > > > > 
> > > > > > > I'm asking about the MMIO address space.  The BAR is a register
> > > > > > > in config space.  AFAICT, clearing P2SBC_HIDE_BYTE makes that
> > > > > > > BAR visible.  The BAR describes a region of PCI address space.
> > > > > > > It looks like setting P2SBC_HIDE_BIT makes the BAR disappear
> > > > > > > from config space, but it sounds like the PCI address space
> > > > > > > *described* by the BAR is still claimed by the device.  If the
> > > > > > > device didn't respond to that MMIO space, you would have no
> > > > > > > reason to read the BAR at all.
> > > > > > > 
> > > > > > > So what keeps the PCI core from assigning that MMIO space to
> > > > > > > another device?
> > > > > > 
> > > > > > The device will respond to MMIO while being hidden. I am afraid
> > > > > > nothing stops a collision, except for the assumption that the BIOS
> > > > > > is always right and PCI devices never get remapped. But just
> > > > > > guessing here.
> > > > > > 
> > > > > > I have seen devices with coreboot having the P2SB visible, and
> > > > > > most likely relocatable. Making it visible in Linux and not hiding
> > > > > > it again might work, but probably only as long as Linux will not
> > > > > > relocate it.  Which i am afraid might seriously upset the BIOS,
> > > > > > depending on what a device does with those GPIOs and which parts
> > > > > > are implemented in the BIOS.
> > > > > 
> > > > > So the question is, do we have knobs in PCI core to mark device
> > > > > fixes in terms of BARs, no relocation must be applied, no other
> > > > > devices must have the region?
> > > > 
> > > > I think the closest thing is the IORESOURCE_PCI_FIXED bit that we use
> > > > for things that must not be moved.  Generally PCI resources are
> > > > associated with a pci_dev, and we set IORESOURCE_PCI_FIXED for BARs,
> > > > e.g., dev->resource[n].  We do that for IDE legacy regions (see
> > > > LEGACY_IO_RESOURCE), Langwell devices (pci_fixed_bar_fixup()),
> > > > "enhanced allocation" (pci_ea_flags()), and some quirks (quirk_io()).
> > > > 
> > > > In your case, the device is hidden so it doesn't respond to config
> > > > accesses, so there is no pci_dev for it.
> > > 
> > > Yes, and the idea is to unhide it on the early stage.
> > > Would it be possible to quirk it to fix the IO resources?
> > 
> > If I read your current patch right, it unhides the device, reads the
> > BAR, then hides the device again.  I didn't see that it would create a
> > pci_dev for it.
> > 
> > If you unhide it and then enumerate it normally (and mark the BAR as
> > IORESOURCE_PCI_FIXED to make sure we never move it), that might work.
> > Then there should be a pci_dev for it, and it would then show up in
> > sysfs, lspci, etc.  And we should insert the BAR in iomem_resource, so
> > we should see it in /proc/iomem and we won't accidentally put
> > something else on top of it.
> 
> If the PCI device is present and we have ACPI description for the one or more
> devices (currently pin control), wouldn't be a conflicting resources issue?
> 
> When would be the suitable place to avoid that?

Given another thought on that and I think we can't unhide entire P2SB due to
possible ACPI tables present which may or may not fully or partially describe
devices behind that bridge, so, I would stick with current approach.

> > > > resource, fills it in, sets IORESOURCE_PCI_FIXED, and does something
> > > > similar to pci_claim_resource()?

-- 
With Best Regards,
Andy Shevchenko



^ permalink raw reply	[flat|nested] 54+ messages in thread

* Re: [PATCH v1 3/7] PCI: New Primary to Sideband (P2SB) bridge support library
  2021-03-09  1:42       ` Bjorn Helgaas
  2021-03-09  8:42         ` Henning Schild
@ 2021-11-26 15:38         ` Andy Shevchenko
  2021-11-29 21:07           ` Bjorn Helgaas
  1 sibling, 1 reply; 54+ messages in thread
From: Andy Shevchenko @ 2021-11-26 15:38 UTC (permalink / raw)
  To: Bjorn Helgaas
  Cc: Wolfram Sang, Jean Delvare, Lee Jones, Tan Jui Nee, Jim Quinlan,
	Jonathan Yong, Bjorn Helgaas, linux-kernel, linux-i2c, linux-pci,
	Jean Delvare, Peter Tyser, hdegoede, henning.schild

On Mon, Mar 08, 2021 at 07:42:21PM -0600, Bjorn Helgaas wrote:
> On Mon, Mar 08, 2021 at 09:16:50PM +0200, Andy Shevchenko wrote:
> > On Mon, Mar 08, 2021 at 12:52:12PM -0600, Bjorn Helgaas wrote:
> > > On Mon, Mar 08, 2021 at 02:20:16PM +0200, Andy Shevchenko wrote:
> > > > From: Jonathan Yong <jonathan.yong@intel.com>
> > > > 
> > > > There is already one and at least one more user is coming which
> > > > requires an access to Primary to Sideband bridge (P2SB) in order to
> > > > get IO or MMIO bar hidden by BIOS. Create a library to access P2SB
> > > > for x86 devices.
> > > 
> > > Can you include a spec reference?
> > 
> > I'm not sure I have a public link to the spec. It's the 100 Series PCH [1].
> > The document number to look for is 546955 [2] and there actually a bit of
> > information about this.
> 
> This link, found by googling for "p2sb bridge", looks like it might
> have relevant public links:
> 
> https://lab.whitequark.org/notes/2017-11-08/accessing-intel-ich-pch-gpios/
> 
> I'd prefer if you could dig out the relevant sections because I really
> don't know how to identify them.

I'm not sure I understand what you would like to see. The information about
P2SB here has confidential tag. I probably can use the document number and cite
couple of paragraphs from it. Would it be sufficient?

> > > I'm trying to figure out why this
> > > belongs in drivers/pci/.  It looks very device-specific.
> > 
> > Because it's all about access to PCI configuration spaces of the (hidden)
> > devices.
> 
> The PCI core generally doesn't deal with device-specific config
> registers.

The location is purely based on practical reason, after the fact that P2SB
is a PCI device, of deduplicating BAR decoding code. I can easily move this
outside of PCI subsystem, but I will need to export this function instead.
Would it work for you?

> > [1]: https://ark.intel.com/content/www/us/en/ark/products/series/98456/intel-100-series-desktop-chipsets.html
> > [2]: https://medium.com/@jacksonchen_43335/bios-gpio-p2sb-70e9b829b403

...

> The code suggests that a register on this device controls whether a
> different device is visible in config space.  I think it will be
> better if we can describe what's happening.

Actually it seems incorrect assumption (while it works by some reason).
I have to double test this.

From the doc:

"The P2SB is enumerated as a normal PCI device. ...
Writing a 1 to the P2SBC.HIDE field in the P2SB PCI Configuration space
hides the device; writing a 0 to this field, unhides the device."

It clearly states the P2SB PCI configuration space.

Also it looks like Henning pointed out to this by asking why we need too many
parameters to the function.

...

> > > > +	/* Unhide the P2SB device */
> > > > +	pci_bus_write_config_byte(bus, df, 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);
> > > 
> > > I don't get this.  Apparently this normally hidden device is consuming
> > > PCI address space.  The PCI core needs to know about this.  If it
> > > doesn't, the PCI core may assign this space to another device.
> > 
> > Right, it returns all 1:s to any request so PCI core *thinks* it's
> > plugged off (like D3cold or so).
> 
> I'm asking about the MMIO address space.

> The BAR is a register in
> config space.  AFAICT, clearing P2SBC_HIDE_BYTE makes that BAR
> visible.  The BAR describes a region of PCI address space.  It looks
> like setting P2SBC_HIDE_BIT makes the BAR disappear from config space,
> but it sounds like the PCI address space *described* by the BAR is
> still claimed by the device.  If the device didn't respond to that
> MMIO space, you would have no reason to read the BAR at all.
> 
> So what keeps the PCI core from assigning that MMIO space to another
> device?
> 
> This all sounds quite irregular from the point of view of the PCI
> core.  If a device responds to address space that is not described by
> a standard PCI BAR, or by an EA capability, or by one of the legacy
> VGA or IDE exceptions, we have a problem.  That space must be
> described *somehow* in a generic way, e.g., ACPI or similar.
> 
> What happens if CONFIG_PCI_P2SB is unset?  The device doesn't know
> that, and if it is still consuming MMIO address space that we don't
> know about, that's a problem.

Yeah, Henning already answered on this and I believe that nothing prevents OS
to try that addresses for other PCI devices, except the memory region
reservation (by ACPI and / or e820 meaning). It means that we rely on firmware
to do the right thing if it hides the P2SB bar.

And at the same time P2SB bar is used as a part of telling OS where the *fixed*
16Mb region of MMIO is located.

> > > > +	/* Hide the P2SB device */
> > > > +	pci_bus_write_config_byte(bus, df, P2SBC_HIDE_BYTE, P2SBC_HIDE_BIT);

-- 
With Best Regards,
Andy Shevchenko



^ permalink raw reply	[flat|nested] 54+ messages in thread

* Re: [rfc, PATCH v1 0/7] PCI: introduce p2sb helper
  2021-03-08 12:20 [rfc, PATCH v1 0/7] PCI: introduce p2sb helper Andy Shevchenko
                   ` (8 preceding siblings ...)
  2021-06-10  9:02 ` Henning Schild
@ 2021-11-26 15:43 ` Andy Shevchenko
  9 siblings, 0 replies; 54+ messages in thread
From: Andy Shevchenko @ 2021-11-26 15:43 UTC (permalink / raw)
  To: Wolfram Sang, Jean Delvare, Lee Jones, Tan Jui Nee, Jim Quinlan,
	Jonathan Yong, Bjorn Helgaas, linux-kernel, linux-i2c, linux-pci
  Cc: Jean Delvare, Peter Tyser, hdegoede, henning.schild

On Mon, Mar 08, 2021 at 02:20:13PM +0200, Andy Shevchenko wrote:
> There are a few users and even at least one more is coming
> that would like to utilize p2sb mechanisms like hide/unhide
> a device from 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.
> 
> Please, comment on the approach and individual patches.
> 
> (Since it's cross subsystem, the PCI seems like a main one and
>  I think it makes sense to route it thru it with immutable tag
>  or branch provided for the others).

TWIMC, after refreshing (a bit) my memories on this thread, I think the roadmap
may look like the following:

1) exporting necessary APIs from PCI core to avoid code dup;
2) moving pci-p2sb.c out from PCI to PDx86 where it seems naturally fit;
3) addressing comments on patches that are not going to change their location /
functionality;
4) adding tags, etc.

Any objections?

Meanwhile I will try to setup a machine with ACPI tables to test the code if
they have not been provided.

-- 
With Best Regards,
Andy Shevchenko



^ permalink raw reply	[flat|nested] 54+ messages in thread

* Re: [PATCH v1 3/7] PCI: New Primary to Sideband (P2SB) bridge support library
  2021-11-26 15:38         ` Andy Shevchenko
@ 2021-11-29 21:07           ` Bjorn Helgaas
  2021-12-08 17:51             ` Andy Shevchenko
  0 siblings, 1 reply; 54+ messages in thread
From: Bjorn Helgaas @ 2021-11-29 21:07 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: Wolfram Sang, Jean Delvare, Lee Jones, Tan Jui Nee, Jim Quinlan,
	Jonathan Yong, Bjorn Helgaas, linux-kernel, linux-i2c, linux-pci,
	Jean Delvare, Peter Tyser, hdegoede, henning.schild

On Fri, Nov 26, 2021 at 05:38:46PM +0200, Andy Shevchenko wrote:
> On Mon, Mar 08, 2021 at 07:42:21PM -0600, Bjorn Helgaas wrote:
> > On Mon, Mar 08, 2021 at 09:16:50PM +0200, Andy Shevchenko wrote:
> > > On Mon, Mar 08, 2021 at 12:52:12PM -0600, Bjorn Helgaas wrote:
> > > > On Mon, Mar 08, 2021 at 02:20:16PM +0200, Andy Shevchenko wrote:
> > > > > From: Jonathan Yong <jonathan.yong@intel.com>
> > > > > 
> > > > > There is already one and at least one more user is coming which
> > > > > requires an access to Primary to Sideband bridge (P2SB) in order to
> > > > > get IO or MMIO bar hidden by BIOS. Create a library to access P2SB
> > > > > for x86 devices.
> > > > 
> > > > Can you include a spec reference?
> > > 
> > > I'm not sure I have a public link to the spec. It's the 100 Series PCH [1].
> > > The document number to look for is 546955 [2] and there actually a bit of
> > > information about this.
> > 
> > This link, found by googling for "p2sb bridge", looks like it might
> > have relevant public links:
> > 
> > https://lab.whitequark.org/notes/2017-11-08/accessing-intel-ich-pch-gpios/
> > 
> > I'd prefer if you could dig out the relevant sections because I really
> > don't know how to identify them.
> 
> I'm not sure I understand what you would like to see. The
> information about P2SB here has confidential tag. I probably can use
> the document number and cite couple of paragraphs from it. Would it
> be sufficient?

This patch proposes to add drivers/pci/pci-p2sb.c.  Things in
drivers/pci/ should generally be documented via PCI-SIG specs to make
them maintainable.  pci-p2sb.c is clearly x86- and Intel-specific.
Maybe arch/x86/pci/ would be a better place for it?

If I were to maintain it under drivers/pci/, I would want some
description about P2SBC_HIDE_BYTE, which device's config space it is
in (apparently 0d.0 of some CPU), and which device it hides/unhides
(apparently some device X other than the CPU).

> > The code suggests that a register on this device controls whether a
> > different device is visible in config space.  I think it will be
> > better if we can describe what's happening.
> 
> Actually it seems incorrect assumption (while it works by some reason).
> I have to double test this.
> 
> From the doc:
> 
> "The P2SB is enumerated as a normal PCI device. ...
> Writing a 1 to the P2SBC.HIDE field in the P2SB PCI Configuration space
> hides the device; writing a 0 to this field, unhides the device."
> 
> It clearly states the P2SB PCI configuration space.

It clearly says P2SBC.HIDE is in P2SB config space.  But I think it's
talking about hiding/unhiding a device other than P2SB.

This patch suggests the P2SB device is at PCI_DEVFN(13, 0), and the
lpc_ich_init_spi() patch suggests there's a SPI controller at
PCI_DEVFN(13, 2).  And apparently P2SBC_HIDE_BIT in PCI_DEVFN(13, 0)
determines whether PCI_DEVFN(13, 2) appears in config space.

So it sounds like P2SB is always visible in config space and is not
itself a "bridge".  The SPI controller, which *is* a bridge from PCI
to SPI, appears at PCI_DEVFN(13, 2) in config space when
P2SBC_HIDE_BIT is cleared.

> > This all sounds quite irregular from the point of view of the PCI
> > core.  If a device responds to address space that is not described by
> > a standard PCI BAR, or by an EA capability, or by one of the legacy
> > VGA or IDE exceptions, we have a problem.  That space must be
> > described *somehow* in a generic way, e.g., ACPI or similar.
> > 
> > What happens if CONFIG_PCI_P2SB is unset?  The device doesn't know
> > that, and if it is still consuming MMIO address space that we don't
> > know about, that's a problem.
> 
> Yeah, Henning already answered on this and I believe that nothing
> prevents OS to try that addresses for other PCI devices, except the
> memory region reservation (by ACPI and / or e820 meaning). It means
> that we rely on firmware to do the right thing if it hides the P2SB
> bar.
> 
> And at the same time P2SB bar is used as a part of telling OS where
> the *fixed* 16Mb region of MMIO is located.

If the SPI controller consumes PCI address space, that space must be
discoverable by standard PCI enumeration and BAR sizing.

If the BIOS hides the device, I assume that means it does not respond
in config space and does not consume MMIO space.  That's fine.

If the BIOS hides the device and the OS unhides it, the unhide should
happen before PCI device enumeration, so the PCI core will find the
device and learn about the MMIO space it consumes.

Bjorn

^ permalink raw reply	[flat|nested] 54+ messages in thread

* Re: [PATCH v1 3/7] PCI: New Primary to Sideband (P2SB) bridge support library
  2021-11-29 21:07           ` Bjorn Helgaas
@ 2021-12-08 17:51             ` Andy Shevchenko
  0 siblings, 0 replies; 54+ messages in thread
From: Andy Shevchenko @ 2021-12-08 17:51 UTC (permalink / raw)
  To: Bjorn Helgaas
  Cc: Wolfram Sang, Jean Delvare, Lee Jones, Tan Jui Nee, Jim Quinlan,
	Jonathan Yong, Bjorn Helgaas, linux-kernel, linux-i2c, linux-pci,
	Jean Delvare, Peter Tyser, hdegoede, henning.schild

On Mon, Nov 29, 2021 at 03:07:05PM -0600, Bjorn Helgaas wrote:
> On Fri, Nov 26, 2021 at 05:38:46PM +0200, Andy Shevchenko wrote:
> > On Mon, Mar 08, 2021 at 07:42:21PM -0600, Bjorn Helgaas wrote:
> > > On Mon, Mar 08, 2021 at 09:16:50PM +0200, Andy Shevchenko wrote:
> > > > On Mon, Mar 08, 2021 at 12:52:12PM -0600, Bjorn Helgaas wrote:
> > > > > On Mon, Mar 08, 2021 at 02:20:16PM +0200, Andy Shevchenko wrote:
> > > > > > From: Jonathan Yong <jonathan.yong@intel.com>
> > > > > > 
> > > > > > There is already one and at least one more user is coming which
> > > > > > requires an access to Primary to Sideband bridge (P2SB) in order to
> > > > > > get IO or MMIO bar hidden by BIOS. Create a library to access P2SB
> > > > > > for x86 devices.
> > > > > 
> > > > > Can you include a spec reference?
> > > > 
> > > > I'm not sure I have a public link to the spec. It's the 100 Series PCH [1].
> > > > The document number to look for is 546955 [2] and there actually a bit of
> > > > information about this.
> > > 
> > > This link, found by googling for "p2sb bridge", looks like it might
> > > have relevant public links:
> > > 
> > > https://lab.whitequark.org/notes/2017-11-08/accessing-intel-ich-pch-gpios/
> > > 
> > > I'd prefer if you could dig out the relevant sections because I really
> > > don't know how to identify them.
> > 
> > I'm not sure I understand what you would like to see. The
> > information about P2SB here has confidential tag. I probably can use
> > the document number and cite couple of paragraphs from it. Would it
> > be sufficient?
> 
> This patch proposes to add drivers/pci/pci-p2sb.c.  Things in
> drivers/pci/ should generally be documented via PCI-SIG specs to make
> them maintainable.  pci-p2sb.c is clearly x86- and Intel-specific.
> Maybe arch/x86/pci/ would be a better place for it?

It's not CPU core, so x86 maintainers for sure be happy to reject this.
The only fit I see for that is PDx86.

> If I were to maintain it under drivers/pci/, I would want some
> description about P2SBC_HIDE_BYTE, which device's config space it is
> in (apparently 0d.0 of some CPU), and which device it hides/unhides
> (apparently some device X other than the CPU).

Since there is publicly (leaked?) available descriptions of it, it's good
to add documentation to the series wherever it will land to.

> > > The code suggests that a register on this device controls whether a
> > > different device is visible in config space.  I think it will be
> > > better if we can describe what's happening.
> > 
> > Actually it seems incorrect assumption (while it works by some reason).
> > I have to double test this.
> > 
> > From the doc:
> > 
> > "The P2SB is enumerated as a normal PCI device. ...
> > Writing a 1 to the P2SBC.HIDE field in the P2SB PCI Configuration space
> > hides the device; writing a 0 to this field, unhides the device."
> > 
> > It clearly states the P2SB PCI configuration space.
> 
> It clearly says P2SBC.HIDE is in P2SB config space.  But I think it's
> talking about hiding/unhiding a device other than P2SB.

> This patch suggests the P2SB device is at PCI_DEVFN(13, 0), and the
> lpc_ich_init_spi() patch suggests there's a SPI controller at
> PCI_DEVFN(13, 2).  And apparently P2SBC_HIDE_BIT in PCI_DEVFN(13, 0)
> determines whether PCI_DEVFN(13, 2) appears in config space.

Ha, now I can answer to this. P2SB is function 0 of the device, which means
that when it's absent the whole device functions are absent. When you enable
function 0, it enables all devices behind it (which is according to the PCI
requirements).

> So it sounds like P2SB is always visible in config space

Determine "visible" here. In terms of PCI software protocol communication it's
not ("hidden").  In terms of hardware access, the writes to this device are
filtered based on this "hidden"-"unhidden" policy, which declares that one bit
in one register is always available, that's it.

> and is not
> itself a "bridge".

It's not.

> The SPI controller, which *is* a bridge from PCI
> to SPI, appears at PCI_DEVFN(13, 2) in config space when
> P2SBC_HIDE_BIT is cleared.

Yes, as PCI talks about. No function can be available (enabled) until function
0 is enabled. It's a side effect here.

> > > This all sounds quite irregular from the point of view of the PCI
> > > core.  If a device responds to address space that is not described by
> > > a standard PCI BAR, or by an EA capability, or by one of the legacy
> > > VGA or IDE exceptions, we have a problem.  That space must be
> > > described *somehow* in a generic way, e.g., ACPI or similar.
> > > 
> > > What happens if CONFIG_PCI_P2SB is unset?  The device doesn't know
> > > that, and if it is still consuming MMIO address space that we don't
> > > know about, that's a problem.
> > 
> > Yeah, Henning already answered on this and I believe that nothing
> > prevents OS to try that addresses for other PCI devices, except the
> > memory region reservation (by ACPI and / or e820 meaning). It means
> > that we rely on firmware to do the right thing if it hides the P2SB
> > bar.
> > 
> > And at the same time P2SB bar is used as a part of telling OS where
> > the *fixed* 16Mb region of MMIO is located.
> 
> If the SPI controller consumes PCI address space, that space must be
> discoverable by standard PCI enumeration and BAR sizing.

I have checked that memory is even supplied as reserved even without device
being visible. Yes, agree on this.

> If the BIOS hides the device, I assume that means it does not respond
> in config space and does not consume MMIO space.  That's fine.

Not really. That MMIO space (behind P2SB, I'm not talking about this
SPI special case) is kinda hard coded to the devices. I haven't clarified
yet if the high part of the BAR can be relocated.

> If the BIOS hides the device and the OS unhides it, the unhide should
> happen before PCI device enumeration, so the PCI core will find the
> device and learn about the MMIO space it consumes.

Yep.

-- 
With Best Regards,
Andy Shevchenko



^ permalink raw reply	[flat|nested] 54+ messages in thread

* Re: [PATCH v1 7/7] i2c: i801: convert to use common P2SB accessor
  2021-03-10 14:51   ` Jean Delvare
@ 2021-12-21 15:08     ` Andy Shevchenko
  0 siblings, 0 replies; 54+ messages in thread
From: Andy Shevchenko @ 2021-12-21 15:08 UTC (permalink / raw)
  To: Jean Delvare
  Cc: Wolfram Sang, Lee Jones, Tan Jui Nee, Jim Quinlan, Jonathan Yong,
	Bjorn Helgaas, linux-kernel, linux-i2c, linux-pci, Peter Tyser,
	hdegoede, henning.schild

On Wed, Mar 10, 2021 at 03:51:45PM +0100, Jean Delvare wrote:
> On Mon,  8 Mar 2021 14:20:20 +0200, Andy Shevchenko wrote:

...

> > -	res->end = res->start + 3;
> > -	res->flags = IORESOURCE_MEM;
> > +		res->start += SBREG_SMBCTRL;
> 
> I can't see why you no longer set res->end and res->flags here. I can
> imagine that pci_p2sb_bar() may have set the flags for us, but not that
> ->end is still correct after you fixed up ->start. Am I missing
> something?

Good catch of the res->end! But flags actually may be MEM64, which the
original code doesn't properly handle.

...

> >  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)),
> >  	{}
> >  };
> 
> Any reason why this is added in this patch instead of [3/7] (PCI: New
> Primary to Sideband (P2SB) bridge support library)?

Filling this on demand, no user no entry. I think it's how we assume the code
to be applied in the kernel.

-- 
With Best Regards,
Andy Shevchenko



^ permalink raw reply	[flat|nested] 54+ messages in thread

end of thread, other threads:[~2021-12-21 15:10 UTC | newest]

Thread overview: 54+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-03-08 12:20 [rfc, PATCH v1 0/7] PCI: introduce p2sb helper Andy Shevchenko
2021-03-08 12:20 ` [PATCH v1 1/7] PCI: Introduce pci_bus_*() printing macros when device is not available Andy Shevchenko
2021-03-10 14:57   ` Jean Delvare
2021-03-08 12:20 ` [PATCH v1 2/7] PCI: Convert __pci_read_base() to __pci_bus_read_base() Andy Shevchenko
2021-03-10 17:14   ` Christoph Hellwig
2021-03-08 12:20 ` [PATCH v1 3/7] PCI: New Primary to Sideband (P2SB) bridge support library Andy Shevchenko
2021-03-08 18:52   ` Bjorn Helgaas
2021-03-08 19:16     ` Andy Shevchenko
2021-03-09  1:42       ` Bjorn Helgaas
2021-03-09  8:42         ` Henning Schild
2021-04-01 15:45           ` Andy Shevchenko
2021-04-01 16:42             ` Bjorn Helgaas
2021-04-01 18:23               ` Andy Shevchenko
2021-04-01 18:44                 ` Bjorn Helgaas
2021-07-12 12:15                   ` Andy Shevchenko
2021-11-26 15:10                     ` Andy Shevchenko
2021-04-02 13:09           ` Enrico Weigelt, metux IT consult
2021-04-06 13:40             ` Henning Schild
2021-07-12 12:11               ` Andy Shevchenko
2021-11-26 15:38         ` Andy Shevchenko
2021-11-29 21:07           ` Bjorn Helgaas
2021-12-08 17:51             ` Andy Shevchenko
2021-03-13  9:45   ` Henning Schild
2021-04-01 15:43     ` Andy Shevchenko
2021-04-01 18:06       ` Mika Westerberg
2021-04-01 18:22         ` Andy Shevchenko
2021-04-01 18:32           ` Mika Westerberg
2021-07-12 12:13             ` Andy Shevchenko
2021-03-08 12:20 ` [PATCH v1 4/7] mfd: lpc_ich: Factor out lpc_ich_enable_spi_write() Andy Shevchenko
2021-03-10 10:29   ` Lee Jones
2021-03-08 12:20 ` [PATCH v1 5/7] mfd: lpc_ich: Switch to generic pci_p2sb_bar() Andy Shevchenko
2021-03-10 10:35   ` Lee Jones
2021-03-10 12:05     ` Andy Shevchenko
2021-03-10 12:57       ` Lee Jones
2021-03-08 12:20 ` [PATCH v1 6/7] mfd: lpc_ich: Add support for pinctrl in non-ACPI system Andy Shevchenko
2021-03-10 10:27   ` Lee Jones
2021-04-12 16:01   ` Henning Schild
2021-04-12 16:40     ` Henning Schild
2021-04-12 16:59       ` Andy Shevchenko
2021-04-12 17:16         ` Henning Schild
2021-04-12 17:34           ` Andy Shevchenko
2021-04-13  6:47             ` Henning Schild
2021-04-12 16:51     ` Andy Shevchenko
2021-04-12 17:27       ` Henning Schild
2021-04-12 17:41         ` Andy Shevchenko
2021-03-08 12:20 ` [PATCH v1 7/7] i2c: i801: convert to use common P2SB accessor Andy Shevchenko
2021-03-10 14:51   ` Jean Delvare
2021-12-21 15:08     ` Andy Shevchenko
2021-03-13  9:25 ` [rfc, PATCH v1 0/7] PCI: introduce p2sb helper Henning Schild
2021-06-10  9:02 ` Henning Schild
2021-06-10 10:14   ` Andy Shevchenko
2021-06-10 13:48     ` Henning Schild
2021-06-10 14:04       ` Andy Shevchenko
2021-11-26 15:43 ` Andy Shevchenko

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).