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
Factor out duplicate code to lpc_ich_enable_spi_write() helper function.
Signed-off-by: Andy Shevchenko <[email protected]>
---
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
From: Tan Jui Nee <[email protected]>
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 <[email protected]>
Signed-off-by: Andy Shevchenko <[email protected]>
---
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 <[email protected]>
*
@@ -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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
---
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, ®ion);
- pcibios_resource_to_bus(dev->bus, &inverted_region, res);
+ pcibios_bus_to_resource(bus, res, ®ion);
+ 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
On Mon, 08 Mar 2021, Andy Shevchenko wrote:
> From: Tan Jui Nee <[email protected]>
>
> 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 <[email protected]>
> Signed-off-by: Andy Shevchenko <[email protected]>
> ---
> 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 <[email protected]>
> *
> @@ -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
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 <[email protected]>
> ---
> 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 <[email protected]>
--
Lee Jones [李琼斯]
Senior Technical Lead - Developer Services
Linaro.org │ Open source software for Arm SoCs
Follow Linaro: Facebook | Twitter | Blog
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 <[email protected]>
> ---
> 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 <[email protected]>
--
Lee Jones [李琼斯]
Senior Technical Lead - Developer Services
Linaro.org │ Open source software for Arm SoCs
Follow Linaro: Facebook | Twitter | Blog
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 <[email protected]>
> > ---
> > 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 <[email protected]>
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/[email protected]/T/#u
[2]: https://lore.kernel.org/lkml/[email protected]/T/#u
--
With Best Regards,
Andy Shevchenko
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 <[email protected]>
> > > ---
> > > 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 <[email protected]>
>
> 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/[email protected]/T/#u
> [2]: https://lore.kernel.org/lkml/[email protected]/T/#u
>
--
Lee Jones [李琼斯]
Senior Technical Lead - Developer Services
Linaro.org │ Open source software for Arm SoCs
Follow Linaro: Facebook | Twitter | Blog
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 <[email protected]>
> ---
> 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
> +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.
Am Mon, 8 Mar 2021 14:20:13 +0200
schrieb Andy Shevchenko <[email protected]>:
> 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 <[email protected]>
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
>
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 <[email protected]>:
> Am Mon, 8 Mar 2021 14:20:19 +0200
> schrieb Andy Shevchenko <[email protected]>:
>
> > From: Tan Jui Nee <[email protected]>
> >
> > 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 <[email protected]>
> > Signed-off-by: Andy Shevchenko <[email protected]>
> > ---
> > 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 <[email protected]>
> > *
> > @@ -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)
>
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 <[email protected]>:
>
> > From: Tan Jui Nee <[email protected]>
> >
> > 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
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
Am Mon, 12 Apr 2021 19:59:05 +0300
schrieb Andy Shevchenko <[email protected]>:
> 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.
>
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 <[email protected]>:
> > 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
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 <[email protected]>:
> > 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 <[email protected]>:
...
> > > > +#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
Am Mon, 8 Mar 2021 14:20:19 +0200
schrieb Andy Shevchenko <[email protected]>:
> From: Tan Jui Nee <[email protected]>
>
> 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 <[email protected]>
> Signed-off-by: Andy Shevchenko <[email protected]>
> ---
> 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 <[email protected]>
> *
> @@ -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)
Am Mon, 12 Apr 2021 19:51:42 +0300
schrieb Andy Shevchenko <[email protected]>:
> 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 <[email protected]>:
> >
> > > From: Tan Jui Nee <[email protected]>
> > >
> > > 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
Am Mon, 12 Apr 2021 20:34:45 +0300
schrieb Andy Shevchenko <[email protected]>:
> 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 <[email protected]>:
> > > 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.
>
Am Mon, 8 Mar 2021 14:20:13 +0200
schrieb Andy Shevchenko <[email protected]>:
> 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
>
On Thu, Jun 10, 2021 at 12:14 PM Henning Schild
<[email protected]> wrote:
>
> Am Mon, 8 Mar 2021 14:20:13 +0200
> schrieb Andy Shevchenko <[email protected]>:
>
> > 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
Am Thu, 10 Jun 2021 13:14:49 +0300
schrieb Andy Shevchenko <[email protected]>:
> On Thu, Jun 10, 2021 at 12:14 PM Henning Schild
> <[email protected]> wrote:
> >
> > Am Mon, 8 Mar 2021 14:20:13 +0200
> > schrieb Andy Shevchenko <[email protected]>:
> >
> > > 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).
>
>
On Thu, Jun 10, 2021 at 4:48 PM Henning Schild
<[email protected]> wrote:
>
> Am Thu, 10 Jun 2021 13:14:49 +0300
> schrieb Andy Shevchenko <[email protected]>:
>
> > On Thu, Jun 10, 2021 at 12:14 PM Henning Schild
> > <[email protected]> wrote:
> > >
> > > Am Mon, 8 Mar 2021 14:20:13 +0200
> > > schrieb Andy Shevchenko <[email protected]>:
> > >
> > > > 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
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
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