[PATCH v2 0/3] pinctrl/broxton: enable platform device in the absent of ACPI enumeration

[PATCH v2 0/3] pinctrl/broxton: enable platform device in the absent of ACPI enumeration

[Tan Jui Nee]

Hi,
The patches are to cater the need for non-ACPI system whereby
a platform device has to be created in order to bind with
Apollo Lake Pinctrl GPIO platform driver.

The MMIO BAR is accessed over the Primary to Sideband bridge
(P2SB). Since the BIOS prevents the P2SB device from being
enumerated by the PCI subsystem, so we need to hide/unhide P2SB
to lookup the P2SB BAR and pass the PCI BAR address to the gpio
platform driver.

All these three patches have dependencies on each other.

Changes from V1:
	- Add new config option CONFIG_X86_INTEL_NON_ACPI and "select PINCTRL"
	  to fix kbuildbot error

Andy Shevchenko (1):
  x86/platform/p2sb: New Primary to Sideband bridge support driver for
    Intel SOC's

Tan Jui Nee (2):
  pinctrl/broxton: enable platform device in the absent of ACPI
    enumeration
  mfd: lpc_ich: Add support for Intel Apollo Lake GPIO pinctrl in
    non-ACPI system

 arch/x86/Kconfig                        |  14 ++++
 arch/x86/include/asm/p2sb.h             |  27 +++++++
 arch/x86/platform/intel/Makefile        |   1 +
 arch/x86/platform/intel/p2sb.c          |  99 ++++++++++++++++++++++++
 drivers/mfd/Kconfig                     |   3 +-
 drivers/mfd/lpc_ich.c                   | 128 ++++++++++++++++++++++++++++++++
 drivers/pinctrl/intel/pinctrl-broxton.c |  43 ++++++++---
 7 files changed, 302 insertions(+), 13 deletions(-)
 create mode 100644 arch/x86/include/asm/p2sb.h
 create mode 100644 arch/x86/platform/intel/p2sb.c

-- 
1.9.1

[PATCH v2 1/3] pinctrl/broxton: enable platform device in the absent of ACPI enumeration

[Tan Jui Nee]

This is to cater the need for non-ACPI system whereby
a platform device has to be created in order to bind
with the Apollo Lake Pinctrl GPIO platform driver.

Signed-off-by: Tan Jui Nee <jui.nee.tan@intel.com>
---
 drivers/pinctrl/intel/pinctrl-broxton.c | 43 ++++++++++++++++++++++++---------
 1 file changed, 31 insertions(+), 12 deletions(-)

diff --git a/drivers/pinctrl/intel/pinctrl-broxton.c b/drivers/pinctrl/intel/pinctrl-broxton.c
index 5979d38..59cb7a6 100644
--- a/drivers/pinctrl/intel/pinctrl-broxton.c
+++ b/drivers/pinctrl/intel/pinctrl-broxton.c
@@ -1,7 +1,7 @@
 /*
  * Intel Broxton SoC pinctrl/GPIO driver
  *
- * Copyright (C) 2015, Intel Corporation
+ * Copyright (C) 2015, 2016 Intel Corporation
  * Author: Mika Westerberg <mika.westerberg@linux.intel.com>
  *
  * This program is free software; you can redistribute it and/or modify
@@ -1003,29 +1003,46 @@ static const struct acpi_device_id bxt_pinctrl_acpi_match[] = {
 };
 MODULE_DEVICE_TABLE(acpi, bxt_pinctrl_acpi_match);
 
+static const struct platform_device_id bxt_pinctrl_platform_ids[] = {
+	{ "apl-pinctrl", (kernel_ulong_t)&apl_pinctrl_soc_data },
+	{ "broxton-pinctrl", (kernel_ulong_t)&bxt_pinctrl_soc_data },
+	{ },
+};
+
 static int bxt_pinctrl_probe(struct platform_device *pdev)
 {
 	const struct intel_pinctrl_soc_data *soc_data = NULL;
 	const struct intel_pinctrl_soc_data **soc_table;
-	const struct acpi_device_id *id;
 	struct acpi_device *adev;
 	int i;
 
 	adev = ACPI_COMPANION(&pdev->dev);
-	if (!adev)
-		return -ENODEV;
+	if (adev) {
+		const struct acpi_device_id *id;
 
-	id = acpi_match_device(bxt_pinctrl_acpi_match, &pdev->dev);
-	if (!id)
-		return -ENODEV;
+		id = acpi_match_device(bxt_pinctrl_acpi_match, &pdev->dev);
+		if (!id)
+			return -ENODEV;
 
-	soc_table = (const struct intel_pinctrl_soc_data **)id->driver_data;
+		soc_table = (const struct intel_pinctrl_soc_data **)
+			id->driver_data;
 
-	for (i = 0; soc_table[i]; i++) {
-		if (!strcmp(adev->pnp.unique_id, soc_table[i]->uid)) {
-			soc_data = soc_table[i];
-			break;
+		for (i = 0; soc_table[i]; i++) {
+			if (!strcmp(adev->pnp.unique_id, soc_table[i]->uid)) {
+				soc_data = soc_table[i];
+				break;
+			}
 		}
+	} else {
+		const struct platform_device_id *pid;
+
+		pid = platform_get_device_id(pdev);
+		if (!pid)
+			return -ENODEV;
+
+		soc_table = (const struct intel_pinctrl_soc_data **)
+			pid->driver_data;
+		soc_data = soc_table[pdev->id];
 	}
 
 	if (!soc_data)
@@ -1047,6 +1064,7 @@ static struct platform_driver bxt_pinctrl_driver = {
 		.acpi_match_table = bxt_pinctrl_acpi_match,
 		.pm = &bxt_pinctrl_pm_ops,
 	},
+	.id_table = bxt_pinctrl_platform_ids,
 };
 
 static int __init bxt_pinctrl_init(void)
@@ -1064,3 +1082,4 @@ module_exit(bxt_pinctrl_exit);
 MODULE_AUTHOR("Mika Westerberg <mika.westerberg@linux.intel.com>");
 MODULE_DESCRIPTION("Intel Broxton SoC pinctrl/GPIO driver");
 MODULE_LICENSE("GPL v2");
+MODULE_ALIAS("platform:broxton-pinctrl");
-- 
1.9.1

Re: [PATCH v2 1/3] pinctrl/broxton: enable platform device in the absent of ACPI enumeration

[Mika Westerberg]

On Tue, Apr 26, 2016 at 02:09:49PM +0800, Tan Jui Nee wrote:
> This is to cater the need for non-ACPI system whereby
> a platform device has to be created in order to bind
> with the Apollo Lake Pinctrl GPIO platform driver.
> 
> Signed-off-by: Tan Jui Nee <jui.nee.tan@intel.com>

Acked-by: Mika Westerberg <mika.westerberg@linux.intel.com>

[PATCH v2 2/3] x86/platform/p2sb: New Primary to Sideband bridge support driver for Intel SOC's

[Tan Jui Nee]

From: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

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

Signed-off-by: Yong, Jonathan <jonathan.yong@intel.com>
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 arch/x86/Kconfig                 | 14 ++++++
 arch/x86/include/asm/p2sb.h      | 27 +++++++++++
 arch/x86/platform/intel/Makefile |  1 +
 arch/x86/platform/intel/p2sb.c   | 99 ++++++++++++++++++++++++++++++++++++++++
 4 files changed, 141 insertions(+)
 create mode 100644 arch/x86/include/asm/p2sb.h
 create mode 100644 arch/x86/platform/intel/p2sb.c

diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
index 2dc18605..589045e 100644
--- a/arch/x86/Kconfig
+++ b/arch/x86/Kconfig
@@ -606,6 +606,20 @@ config IOSF_MBI_DEBUG
 
 	  If you don't require the option or are in doubt, say N.
 
+config X86_INTEL_NON_ACPI
+	bool "Enable support non-ACPI Intel platforms"
+	select PINCTRL
+	---help---
+	  Select this option to enables MMIO BAR access over the P2SB for
+	  non-ACPI Intel SoC platforms. This driver uses the P2SB hide/unhide
+	  mechanism cooperatively to pass the PCI BAR address to the platform
+	  driver, currently GPIO on the following SoC products.
+	   - Apollo Lake
+
+config P2SB
+	tristate
+	depends on PCI
+
 config X86_RDC321X
 	bool "RDC R-321x SoC"
 	depends on X86_32
diff --git a/arch/x86/include/asm/p2sb.h b/arch/x86/include/asm/p2sb.h
new file mode 100644
index 0000000..686e07b
--- /dev/null
+++ b/arch/x86/include/asm/p2sb.h
@@ -0,0 +1,27 @@
+/*
+ * Primary to Sideband bridge (P2SB) access support
+ */
+
+#ifndef P2SB_SYMS_H
+#define P2SB_SYMS_H
+
+#include <linux/ioport.h>
+#include <linux/pci.h>
+
+#if IS_ENABLED(CONFIG_P2SB)
+
+int p2sb_bar(struct pci_dev *pdev, unsigned int devfn,
+	struct resource *res);
+
+#else /* CONFIG_P2SB is not set */
+
+static inline
+int p2sb_bar(struct pci_dev *pdev, unsigned int devfn,
+	struct resource *res)
+{
+	return -ENODEV;
+}
+
+#endif /* CONFIG_P2SB */
+
+#endif /* P2SB_SYMS_H */
diff --git a/arch/x86/platform/intel/Makefile b/arch/x86/platform/intel/Makefile
index b878032..dbf9f10 100644
--- a/arch/x86/platform/intel/Makefile
+++ b/arch/x86/platform/intel/Makefile
@@ -1 +1,2 @@
 obj-$(CONFIG_IOSF_MBI)			+= iosf_mbi.o
+obj-$(CONFIG_P2SB)			+= p2sb.o
diff --git a/arch/x86/platform/intel/p2sb.c b/arch/x86/platform/intel/p2sb.c
new file mode 100644
index 0000000..8be47a4
--- /dev/null
+++ b/arch/x86/platform/intel/p2sb.c
@@ -0,0 +1,99 @@
+/*
+ * Primary to Sideband bridge (P2SB) driver
+ *
+ * Copyright (c) 2016, Intel Corporation.
+ *
+ * Authors: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
+ *			Jonathan Yong <jonathan.yong@intel.com>
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms and conditions of the GNU General Public License,
+ * version 2, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
+ * more details.
+ *
+ */
+
+#include <linux/ioport.h>
+#include <linux/module.h>
+#include <linux/pci.h>
+#include <linux/spinlock.h>
+
+#include <asm/p2sb.h>
+
+#define SBREG_BAR	0x10
+#define SBREG_HIDE	0xe1
+
+static DEFINE_SPINLOCK(p2sb_spinlock);
+
+/*
+ * p2sb_bar - Get Primary to Sideband bridge (P2SB) BAR
+ * @pdev:	PCI device to get PCI bus to communicate with
+ * @devfn:	PCI device and function to communicate with
+ * @res:	resources 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 P2SB BAR.
+ *
+ * Locking is handled by spinlock - cannot sleep.
+ *
+ * Return:
+ * 0 on success or appropriate errno value on error.
+ */
+int p2sb_bar(struct pci_dev *pdev, unsigned int devfn,
+	struct resource *res)
+{
+	u32 base_addr;
+	u64 base64_addr;
+	unsigned long flags;
+
+	if (!res)
+		return -EINVAL;
+
+	spin_lock(&p2sb_spinlock);
+
+	/* Unhide the P2SB device */
+	pci_bus_write_config_byte(pdev->bus, devfn, SBREG_HIDE, 0x00);
+
+	/* Check if device present */
+	pci_bus_read_config_dword(pdev->bus, devfn, 0, &base_addr);
+	if (base_addr == 0xffffffff || base_addr == 0x00000000) {
+		spin_unlock(&p2sb_spinlock);
+		dev_warn(&pdev->dev, "P2SB device access disabled by BIOS?\n");
+		return -ENODEV;
+	}
+
+	/* Get IO or MMIO BAR */
+	pci_bus_read_config_dword(pdev->bus, devfn, SBREG_BAR, &base_addr);
+	if ((base_addr & PCI_BASE_ADDRESS_SPACE) == PCI_BASE_ADDRESS_SPACE_IO) {
+		flags = IORESOURCE_IO;
+		base64_addr = base_addr & PCI_BASE_ADDRESS_IO_MASK;
+	} else {
+		flags = IORESOURCE_MEM;
+		base64_addr = base_addr & PCI_BASE_ADDRESS_MEM_MASK;
+		if (base_addr & PCI_BASE_ADDRESS_MEM_TYPE_64) {
+			flags |= IORESOURCE_MEM_64;
+			pci_bus_read_config_dword(pdev->bus, devfn,
+				SBREG_BAR + 4, &base_addr);
+			base64_addr |= (u64)base_addr << 32;
+		}
+	}
+
+	/* Hide the P2SB device */
+	pci_bus_write_config_byte(pdev->bus, devfn, SBREG_HIDE, 0x01);
+
+	spin_unlock(&p2sb_spinlock);
+
+	/* User provides prefilled resources */
+	res->start += (resource_size_t)base64_addr;
+	res->end += (resource_size_t)base64_addr;
+	res->flags = flags;
+
+	return 0;
+}
+EXPORT_SYMBOL(p2sb_bar);
+
+MODULE_LICENSE("GPL");
-- 
1.9.1

[PATCH v2 3/3] mfd: lpc_ich: Add support for Intel Apollo Lake GPIO pinctrl in non-ACPI system

[Tan Jui Nee]

This driver uses the P2SB hide/unhide mechanism cooperatively
to pass the PCI BAR address to the gpio platform driver.

Signed-off-by: Tan Jui Nee <jui.nee.tan@intel.com>
---
 drivers/mfd/Kconfig   |   3 +-
 drivers/mfd/lpc_ich.c | 128 ++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 130 insertions(+), 1 deletion(-)

diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
index eea61e3..54e595c 100644
--- a/drivers/mfd/Kconfig
+++ b/drivers/mfd/Kconfig
@@ -359,8 +359,9 @@ config MFD_INTEL_QUARK_I2C_GPIO
 
 config LPC_ICH
 	tristate "Intel ICH LPC"
-	depends on PCI
+	depends on X86 && PCI
 	select MFD_CORE
+	select P2SB if X86_INTEL_NON_ACPI
 	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 bd3aa45..5d0cc9b 100644
--- a/drivers/mfd/lpc_ich.c
+++ b/drivers/mfd/lpc_ich.c
@@ -68,6 +68,10 @@
 #include <linux/mfd/core.h>
 #include <linux/mfd/lpc_ich.h>
 #include <linux/platform_data/itco_wdt.h>
+#include <linux/pinctrl/pinctrl.h>
+#include <linux/types.h>
+
+#include <asm/p2sb.h>
 
 #define ACPIBASE		0x40
 #define ACPIBASE_GPE_OFF	0x28
@@ -94,6 +98,19 @@
 #define wdt_mem_res(i) wdt_res(ICH_RES_MEM_OFF, i)
 #define wdt_res(b, i) (&wdt_ich_res[(b) + (i)])
 
+/* Offset data for Apollo Lake GPIO communities */
+#define APL_GPIO_SOUTHWEST_OFFSET	0xc0
+#define APL_GPIO_NORTHWEST_OFFSET	0xc4
+#define APL_GPIO_NORTH_OFFSET		0xc5
+#define APL_GPIO_WEST_OFFSET		0xc7
+
+#define APL_GPIO_SOUTHWEST_END		(43 * 0x8)
+#define APL_GPIO_NORTHWEST_END		(77 * 0x8)
+#define APL_GPIO_NORTH_END		(90 * 0x8)
+#define APL_GPIO_WEST_END		(47 * 0x8)
+
+#define APL_GPIO_IRQ 14
+
 struct lpc_ich_priv {
 	int chipset;
 
@@ -133,6 +150,50 @@ static struct resource gpio_ich_res[] = {
 	},
 };
 
+#ifdef CONFIG_X86_INTEL_NON_ACPI
+static struct resource apl_gpio_io_res[][2] = {
+	{
+		{
+			.start = APL_GPIO_NORTH_OFFSET << 16,
+			.end = (APL_GPIO_NORTH_OFFSET << 16)
+				+ APL_GPIO_NORTH_END,
+		},
+	},
+	{
+		{
+			.start = APL_GPIO_NORTHWEST_OFFSET << 16,
+			.end = (APL_GPIO_NORTHWEST_OFFSET << 16)
+				+ APL_GPIO_NORTHWEST_END,
+		},
+	},
+	{
+		{
+			.start = APL_GPIO_WEST_OFFSET << 16,
+			.end = (APL_GPIO_WEST_OFFSET << 16)
+				+ APL_GPIO_WEST_END,
+		},
+	},
+	{
+		{
+			.start = APL_GPIO_SOUTHWEST_OFFSET << 16,
+			.end = (APL_GPIO_SOUTHWEST_OFFSET << 16)
+				+ APL_GPIO_SOUTHWEST_END,
+		},
+	},
+};
+
+static struct pinctrl_pin_desc apl_pinctrl_pdata;
+
+static struct mfd_cell apl_gpio_devices = {
+	.name = "apl-pinctrl",
+	.num_resources = ARRAY_SIZE(apl_gpio_io_res),
+	.resources = apl_gpio_io_res[1],
+	.pdata_size = sizeof(apl_pinctrl_pdata),
+	.platform_data = &apl_pinctrl_pdata,
+	.ignore_resource_conflicts = true,
+};
+#endif /* CONFIG_X86_INTEL_NON_ACPI */
+
 static struct mfd_cell lpc_ich_wdt_cell = {
 	.name = "iTCO_wdt",
 	.num_resources = ARRAY_SIZE(wdt_ich_res),
@@ -216,6 +277,7 @@ enum lpc_chipsets {
 	LPC_BRASWELL,	/* Braswell SoC */
 	LPC_LEWISBURG,	/* Lewisburg */
 	LPC_9S,		/* 9 Series */
+	LPC_APL,	/* Apollo Lake SoC */
 };
 
 static struct lpc_ich_info lpc_chipset_info[] = {
@@ -531,6 +593,10 @@ static struct lpc_ich_info lpc_chipset_info[] = {
 		.name = "9 Series",
 		.iTCO_version = 2,
 	},
+	[LPC_APL]  = {
+		.name = "Apollo Lake SoC",
+		.iTCO_version = 5,
+	},
 };
 
 /*
@@ -679,6 +745,7 @@ static const struct pci_device_id lpc_ich_ids[] = {
 	{ PCI_VDEVICE(INTEL, 0x3b14), LPC_3420},
 	{ PCI_VDEVICE(INTEL, 0x3b16), LPC_3450},
 	{ PCI_VDEVICE(INTEL, 0x5031), LPC_EP80579},
+	{ PCI_VDEVICE(INTEL, 0x5ae8), LPC_APL},
 	{ PCI_VDEVICE(INTEL, 0x8c40), LPC_LPT},
 	{ PCI_VDEVICE(INTEL, 0x8c41), LPC_LPT},
 	{ PCI_VDEVICE(INTEL, 0x8c42), LPC_LPT},
@@ -1050,6 +1117,64 @@ wdt_done:
 	return ret;
 }
 
+#ifdef CONFIG_X86_INTEL_NON_ACPI
+static int lpc_ich_misc(struct pci_dev *dev, enum lpc_chipsets chipset)
+{
+	unsigned int apl_p2sb = PCI_DEVFN(0x0d, 0);
+	unsigned int i;
+	int ret;
+
+	switch (chipset) {
+	case LPC_APL:
+		/*
+		 * Apollo lake, has not 1, but 4 gpio controllers,
+		 * handle it a bit differently.
+		 */
+
+		for (i = 0; i < ARRAY_SIZE(apl_gpio_io_res); i++) {
+			struct resource *res = apl_gpio_io_res[i];
+
+			apl_gpio_devices.resources = res;
+
+			/* Fill MEM resource */
+			ret = p2sb_bar(dev, apl_p2sb, res++);
+			if (ret)
+				goto warn_continue;
+
+			/* Fill IRQ resource */
+			res->start = APL_GPIO_IRQ;
+			res->end = res->start;
+			res->flags = IORESOURCE_IRQ;
+
+			apl_pinctrl_pdata.name = kasprintf(GFP_KERNEL, "%u",
+				i + 1);
+			if (apl_pinctrl_pdata.name)
+				ret = mfd_add_devices(&dev->dev, i,
+					&apl_gpio_devices, 1, NULL, 0, NULL);
+			else
+				ret = -ENOMEM;
+
+warn_continue:
+			if (ret)
+				dev_warn(&dev->dev,
+					"Failed to add Apollo Lake GPIO %s: %d\n",
+						apl_pinctrl_pdata.name, ret);
+
+			kfree(apl_pinctrl_pdata.name);
+		}
+		break;
+	default:
+		return -ENODEV;
+	}
+	return 0;
+}
+#else
+static inline int lpc_ich_misc(struct pci_dev *dev, enum lpc_chipsets chipset)
+{
+	return -ENODEV;
+}
+#endif /* CONFIG_X86_INTEL_NON_ACPI */
+
 static int lpc_ich_probe(struct pci_dev *dev,
 				const struct pci_device_id *id)
 {
@@ -1093,6 +1218,9 @@ static int lpc_ich_probe(struct pci_dev *dev,
 			cell_added = true;
 	}
 
+	if (!lpc_ich_misc(dev, priv->chipset))
+		cell_added = true;
+
 	/*
 	 * We only care if at least one or none of the cells registered
 	 * successfully.
-- 
1.9.1

Re: [PATCH v2 3/3] mfd: lpc_ich: Add support for Intel Apollo Lake GPIO pinctrl in non-ACPI system

[Lee Jones]

On Tue, 26 Apr 2016, Tan Jui Nee wrote:

> This driver uses the P2SB hide/unhide mechanism cooperatively
> to pass the PCI BAR address to the gpio platform driver.
> 
> Signed-off-by: Tan Jui Nee <jui.nee.tan@intel.com>
> ---
>  drivers/mfd/Kconfig   |   3 +-
>  drivers/mfd/lpc_ich.c | 128 ++++++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 130 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> index eea61e3..54e595c 100644
> --- a/drivers/mfd/Kconfig
> +++ b/drivers/mfd/Kconfig
> @@ -359,8 +359,9 @@ config MFD_INTEL_QUARK_I2C_GPIO
>  
>  config LPC_ICH
>  	tristate "Intel ICH LPC"
> -	depends on PCI
> +	depends on X86 && PCI
>  	select MFD_CORE
> +	select P2SB if X86_INTEL_NON_ACPI
>  	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 bd3aa45..5d0cc9b 100644
> --- a/drivers/mfd/lpc_ich.c
> +++ b/drivers/mfd/lpc_ich.c
> @@ -68,6 +68,10 @@
>  #include <linux/mfd/core.h>
>  #include <linux/mfd/lpc_ich.h>
>  #include <linux/platform_data/itco_wdt.h>
> +#include <linux/pinctrl/pinctrl.h>
> +#include <linux/types.h>
> +
> +#include <asm/p2sb.h>
>  
>  #define ACPIBASE		0x40
>  #define ACPIBASE_GPE_OFF	0x28
> @@ -94,6 +98,19 @@
>  #define wdt_mem_res(i) wdt_res(ICH_RES_MEM_OFF, i)
>  #define wdt_res(b, i) (&wdt_ich_res[(b) + (i)])
>  
> +/* Offset data for Apollo Lake GPIO communities */
> +#define APL_GPIO_SOUTHWEST_OFFSET	0xc0
> +#define APL_GPIO_NORTHWEST_OFFSET	0xc4
> +#define APL_GPIO_NORTH_OFFSET		0xc5
> +#define APL_GPIO_WEST_OFFSET		0xc7
> +
> +#define APL_GPIO_SOUTHWEST_END		(43 * 0x8)
> +#define APL_GPIO_NORTHWEST_END		(77 * 0x8)
> +#define APL_GPIO_NORTH_END		(90 * 0x8)
> +#define APL_GPIO_WEST_END		(47 * 0x8)
> +
> +#define APL_GPIO_IRQ 14
> +
>  struct lpc_ich_priv {
>  	int chipset;
>  
> @@ -133,6 +150,50 @@ static struct resource gpio_ich_res[] = {
>  	},
>  };
>  
> +#ifdef CONFIG_X86_INTEL_NON_ACPI

No, thank you.  No unnecessary #ifery.

> +static struct resource apl_gpio_io_res[][2] = {

The "[][2]" is a warning sign to me.

> +	{
> +		{
> +			.start = APL_GPIO_NORTH_OFFSET << 16,
> +			.end = (APL_GPIO_NORTH_OFFSET << 16)
> +				+ APL_GPIO_NORTH_END,

This is a strange (and complicated) way of calculating register
addresses.  Please simplify.  If in doubt, check out how other
drivers/platforms handle it.

> +		},
> +	},
> +	{
> +		{
> +			.start = APL_GPIO_NORTHWEST_OFFSET << 16,
> +			.end = (APL_GPIO_NORTHWEST_OFFSET << 16)
> +				+ APL_GPIO_NORTHWEST_END,
> +		},
> +	},
> +	{
> +		{
> +			.start = APL_GPIO_WEST_OFFSET << 16,
> +			.end = (APL_GPIO_WEST_OFFSET << 16)
> +				+ APL_GPIO_WEST_END,
> +		},
> +	},
> +	{
> +		{
> +			.start = APL_GPIO_SOUTHWEST_OFFSET << 16,
> +			.end = (APL_GPIO_SOUTHWEST_OFFSET << 16)
> +				+ APL_GPIO_SOUTHWEST_END,
> +		},
> +	},
> +};

Use the DEFINE_RES_* defines from include/linux/ioport.h.

> +static struct pinctrl_pin_desc apl_pinctrl_pdata;

No externs.  Have you ran this through checkpatch.pl?

I don't even see this struct.  Where is it?

> +static struct mfd_cell apl_gpio_devices = {
> +	.name = "apl-pinctrl",
> +	.num_resources = ARRAY_SIZE(apl_gpio_io_res),
> +	.resources = apl_gpio_io_res[1],
> +	.pdata_size = sizeof(apl_pinctrl_pdata),
> +	.platform_data = &apl_pinctrl_pdata,
> +	.ignore_resource_conflicts = true,
> +};
> +#endif /* CONFIG_X86_INTEL_NON_ACPI */
> +
>  static struct mfd_cell lpc_ich_wdt_cell = {
>  	.name = "iTCO_wdt",
>  	.num_resources = ARRAY_SIZE(wdt_ich_res),
> @@ -216,6 +277,7 @@ enum lpc_chipsets {
>  	LPC_BRASWELL,	/* Braswell SoC */
>  	LPC_LEWISBURG,	/* Lewisburg */
>  	LPC_9S,		/* 9 Series */
> +	LPC_APL,	/* Apollo Lake SoC */
>  };
>  
>  static struct lpc_ich_info lpc_chipset_info[] = {
> @@ -531,6 +593,10 @@ static struct lpc_ich_info lpc_chipset_info[] = {
>  		.name = "9 Series",
>  		.iTCO_version = 2,
>  	},
> +	[LPC_APL]  = {
> +		.name = "Apollo Lake SoC",
> +		.iTCO_version = 5,
> +	},
>  };
>  
>  /*
> @@ -679,6 +745,7 @@ static const struct pci_device_id lpc_ich_ids[] = {
>  	{ PCI_VDEVICE(INTEL, 0x3b14), LPC_3420},
>  	{ PCI_VDEVICE(INTEL, 0x3b16), LPC_3450},
>  	{ PCI_VDEVICE(INTEL, 0x5031), LPC_EP80579},
> +	{ PCI_VDEVICE(INTEL, 0x5ae8), LPC_APL},
>  	{ PCI_VDEVICE(INTEL, 0x8c40), LPC_LPT},
>  	{ PCI_VDEVICE(INTEL, 0x8c41), LPC_LPT},
>  	{ PCI_VDEVICE(INTEL, 0x8c42), LPC_LPT},
> @@ -1050,6 +1117,64 @@ wdt_done:
>  	return ret;
>  }
>  
> +#ifdef CONFIG_X86_INTEL_NON_ACPI
> +static int lpc_ich_misc(struct pci_dev *dev, enum lpc_chipsets chipset)
> +{
> +	unsigned int apl_p2sb = PCI_DEVFN(0x0d, 0);

No magic numbers?  Please define them.

> +	unsigned int i;
> +	int ret;
> +
> +	switch (chipset) {

Why a switch?  This would look better if you:

if (chipset != LPC_APL)
   return -ENODEV;

... until you actually *need* a switch.

> +	case LPC_APL:
> +		/*
> +		 * Apollo lake, has not 1, but 4 gpio controllers,
> +		 * handle it a bit differently.
> +		 */
> +
> +		for (i = 0; i < ARRAY_SIZE(apl_gpio_io_res); i++) {
> +			struct resource *res = apl_gpio_io_res[i];
> +
> +			apl_gpio_devices.resources = res;
> +
> +			/* Fill MEM resource */
> +			ret = p2sb_bar(dev, apl_p2sb, res++);
> +			if (ret)
> +				goto warn_continue;

What does this do?

> +			/* Fill IRQ resource */
> +			res->start = APL_GPIO_IRQ;
> +			res->end = res->start;
> +			res->flags = IORESOURCE_IRQ;
> +
> +			apl_pinctrl_pdata.name = kasprintf(GFP_KERNEL, "%u",
> +				i + 1);

All this to call a device "1", "2", etc?

> +			if (apl_pinctrl_pdata.name)
> +				ret = mfd_add_devices(&dev->dev, i,
> +					&apl_gpio_devices, 1, NULL, 0, NULL);

mfd_add_devices() is designed to take a group of devices and register
them all for you.  Calling it once for each separate device you have
is not correct.

> +			else
> +				ret = -ENOMEM;
> +
> +warn_continue:
> +			if (ret)
> +				dev_warn(&dev->dev,
> +					"Failed to add Apollo Lake GPIO %s: %d\n",
> +						apl_pinctrl_pdata.name, ret);
> +
> +			kfree(apl_pinctrl_pdata.name);
> +		}

This code just looks like one big hack to me.

Why don't you just declare the correct amount of resources in
apl_gpio_devices instead of this hodge-podge?

> +		break;
> +	default:
> +		return -ENODEV;
> +	}
> +	return 0;
> +}
> +#else
> +static inline int lpc_ich_misc(struct pci_dev *dev, enum lpc_chipsets chipset)
> +{
> +	return -ENODEV;
> +}
> +#endif /* CONFIG_X86_INTEL_NON_ACPI */

Don't do this in drivers.

>  static int lpc_ich_probe(struct pci_dev *dev,
>  				const struct pci_device_id *id)
>  {
> @@ -1093,6 +1218,9 @@ static int lpc_ich_probe(struct pci_dev *dev,
>  			cell_added = true;
>  	}
>  
> +	if (!lpc_ich_misc(dev, priv->chipset))
> +		cell_added = true;
> +
>  	/*
>  	 * We only care if at least one or none of the cells registered
>  	 * successfully.

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

RE: [PATCH v2 3/3] mfd: lpc_ich: Add support for Intel Apollo Lake GPIO pinctrl in non-ACPI system

[Tan, Jui Nee]

> -----Original Message-----
> From: Lee Jones [mailto:lee.jones@linaro.org]
> Sent: Monday, May 9, 2016 8:25 PM
> To: Tan, Jui Nee <jui.nee.tan@intel.com>
> Cc: mika.westerberg@linux.intel.com; heikki.krogerus@linux.intel.com;
> andriy.shevchenko@linux.intel.com; tglx@linutronix.de;
> mingo@redhat.com; hpa@zytor.com; x86@kernel.org; ptyser@xes-inc.com;
> linux-gpio@vger.kernel.org; linux-kernel@vger.kernel.org; Yong, Jonathan
> <jonathan.yong@intel.com>; Yu, Ong Hock <ong.hock.yu@intel.com>; Voon,
> Weifeng <weifeng.voon@intel.com>; Wan Mohamad, Wan Ahmad Zainie
> <wan.ahmad.zainie.wan.mohamad@intel.com>
> Subject: Re: [PATCH v2 3/3] mfd: lpc_ich: Add support for Intel Apollo Lake
> GPIO pinctrl in non-ACPI system
> 
> On Tue, 26 Apr 2016, Tan Jui Nee wrote:
> 
> > This driver uses the P2SB hide/unhide mechanism cooperatively to pass
> > the PCI BAR address to the gpio platform driver.
> >
> > Signed-off-by: Tan Jui Nee <jui.nee.tan@intel.com>
> > ---
> >  drivers/mfd/Kconfig   |   3 +-
> >  drivers/mfd/lpc_ich.c | 128
> > ++++++++++++++++++++++++++++++++++++++++++++++++++
> >  2 files changed, 130 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig index
> > eea61e3..54e595c 100644
> > --- a/drivers/mfd/Kconfig
> > +++ b/drivers/mfd/Kconfig
> > @@ -359,8 +359,9 @@ config MFD_INTEL_QUARK_I2C_GPIO
> >
> >  config LPC_ICH
> >  	tristate "Intel ICH LPC"
> > -	depends on PCI
> > +	depends on X86 && PCI
> >  	select MFD_CORE
> > +	select P2SB if X86_INTEL_NON_ACPI
> >  	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
> > bd3aa45..5d0cc9b 100644
> > --- a/drivers/mfd/lpc_ich.c
> > +++ b/drivers/mfd/lpc_ich.c
> > @@ -68,6 +68,10 @@
> >  #include <linux/mfd/core.h>
> >  #include <linux/mfd/lpc_ich.h>
> >  #include <linux/platform_data/itco_wdt.h>
> > +#include <linux/pinctrl/pinctrl.h>
> > +#include <linux/types.h>
> > +
> > +#include <asm/p2sb.h>
> >
> >  #define ACPIBASE		0x40
> >  #define ACPIBASE_GPE_OFF	0x28
> > @@ -94,6 +98,19 @@
> >  #define wdt_mem_res(i) wdt_res(ICH_RES_MEM_OFF, i)  #define
> > wdt_res(b, i) (&wdt_ich_res[(b) + (i)])
> >
> > +/* Offset data for Apollo Lake GPIO communities */
> > +#define APL_GPIO_SOUTHWEST_OFFSET	0xc0
> > +#define APL_GPIO_NORTHWEST_OFFSET	0xc4
> > +#define APL_GPIO_NORTH_OFFSET		0xc5
> > +#define APL_GPIO_WEST_OFFSET		0xc7
> > +
> > +#define APL_GPIO_SOUTHWEST_END		(43 * 0x8)
> > +#define APL_GPIO_NORTHWEST_END		(77 * 0x8)
> > +#define APL_GPIO_NORTH_END		(90 * 0x8)
> > +#define APL_GPIO_WEST_END		(47 * 0x8)
> > +
> > +#define APL_GPIO_IRQ 14
> > +
> >  struct lpc_ich_priv {
> >  	int chipset;
> >
> > @@ -133,6 +150,50 @@ static struct resource gpio_ich_res[] = {
> >  	},
> >  };
> >
> > +#ifdef CONFIG_X86_INTEL_NON_ACPI
> 
> No, thank you.  No unnecessary #ifery.
> 
Sorry for the long delay. 
There is complaint by kbuidbot about specific kernel configuration, i.e. 
CONFIG_PINCTRL=n. To solve the issue, I introduced a new config option 
CONFIG_X86_INTEL_NON_ACPI. I don't know the best solution here, or maybe
you can advise something.

> > +static struct resource apl_gpio_io_res[][2] = {
> 
> The "[][2]" is a warning sign to me.
> 
Since there are 4 GPIO community for APL, I have modified something like below.
Please comment. Thanks.
	static struct resource apl_gpio_io_res[4][2] = {

> > +	{
> > +		{
> > +			.start = APL_GPIO_NORTH_OFFSET << 16,
> > +			.end = (APL_GPIO_NORTH_OFFSET << 16)
> > +				+ APL_GPIO_NORTH_END,
> 
> This is a strange (and complicated) way of calculating register addresses.
> Please simplify.  If in doubt, check out how other drivers/platforms handle it.
> 
I have modified to something like below in next patch-set:
	...
	#define APL_GPIO_SOUTHWEST_OFFSET	0xc00000
	...
	#define APL_GPIO_SOUTHWEST_NPIN		43
	...
	static struct resource apl_gpio_io_res[4][2] = {
		DEFINE_RES_MEM_NAMED(APL_GPIO_SOUTHWEST_OFFSET,
			APL_GPIO_SOUTHWEST_NPIN * SZ_8, "apl_pinctrl_sw"),
	...

> > +		},
> > +	},
> > +	{
> > +		{
> > +			.start = APL_GPIO_NORTHWEST_OFFSET << 16,
> > +			.end = (APL_GPIO_NORTHWEST_OFFSET << 16)
> > +				+ APL_GPIO_NORTHWEST_END,
> > +		},
> > +	},
> > +	{
> > +		{
> > +			.start = APL_GPIO_WEST_OFFSET << 16,
> > +			.end = (APL_GPIO_WEST_OFFSET << 16)
> > +				+ APL_GPIO_WEST_END,
> > +		},
> > +	},
> > +	{
> > +		{
> > +			.start = APL_GPIO_SOUTHWEST_OFFSET << 16,
> > +			.end = (APL_GPIO_SOUTHWEST_OFFSET << 16)
> > +				+ APL_GPIO_SOUTHWEST_END,
> > +		},
> > +	},
> > +};
> 
> Use the DEFINE_RES_* defines from include/linux/ioport.h.
> 
I will replace it with DEFINE_RES_MEM_NAMED in my next patch-set.

> > +static struct pinctrl_pin_desc apl_pinctrl_pdata;
> 
> No externs.  Have you ran this through checkpatch.pl?
> 
> I don't even see this struct.  Where is it?
> 
The pinctrl_pin_desc struct is located at include/linux/pinctrl/pinctrl.h and that
is the purpose we need CONFIG_X86_INTEL_NON_ACPI to select
CONFIG_PINCTRL. Else, kbuidbot will complain.

> > +static struct mfd_cell apl_gpio_devices = {
> > +	.name = "apl-pinctrl",
> > +	.num_resources = ARRAY_SIZE(apl_gpio_io_res),
> > +	.resources = apl_gpio_io_res[1],
> > +	.pdata_size = sizeof(apl_pinctrl_pdata),
> > +	.platform_data = &apl_pinctrl_pdata,
> > +	.ignore_resource_conflicts = true,
> > +};
> > +#endif /* CONFIG_X86_INTEL_NON_ACPI */
> > +
> >  static struct mfd_cell lpc_ich_wdt_cell = {
> >  	.name = "iTCO_wdt",
> >  	.num_resources = ARRAY_SIZE(wdt_ich_res), @@ -216,6 +277,7 @@
> enum
> > lpc_chipsets {
> >  	LPC_BRASWELL,	/* Braswell SoC */
> >  	LPC_LEWISBURG,	/* Lewisburg */
> >  	LPC_9S,		/* 9 Series */
> > +	LPC_APL,	/* Apollo Lake SoC */
> >  };
> >
> >  static struct lpc_ich_info lpc_chipset_info[] = { @@ -531,6 +593,10
> > @@ static struct lpc_ich_info lpc_chipset_info[] = {
> >  		.name = "9 Series",
> >  		.iTCO_version = 2,
> >  	},
> > +	[LPC_APL]  = {
> > +		.name = "Apollo Lake SoC",
> > +		.iTCO_version = 5,
> > +	},
> >  };
> >
> >  /*
> > @@ -679,6 +745,7 @@ static const struct pci_device_id lpc_ich_ids[] = {
> >  	{ PCI_VDEVICE(INTEL, 0x3b14), LPC_3420},
> >  	{ PCI_VDEVICE(INTEL, 0x3b16), LPC_3450},
> >  	{ PCI_VDEVICE(INTEL, 0x5031), LPC_EP80579},
> > +	{ PCI_VDEVICE(INTEL, 0x5ae8), LPC_APL},
> >  	{ PCI_VDEVICE(INTEL, 0x8c40), LPC_LPT},
> >  	{ PCI_VDEVICE(INTEL, 0x8c41), LPC_LPT},
> >  	{ PCI_VDEVICE(INTEL, 0x8c42), LPC_LPT}, @@ -1050,6 +1117,64 @@
> > wdt_done:
> >  	return ret;
> >  }
> >
> > +#ifdef CONFIG_X86_INTEL_NON_ACPI
> > +static int lpc_ich_misc(struct pci_dev *dev, enum lpc_chipsets
> > +chipset) {
> > +	unsigned int apl_p2sb = PCI_DEVFN(0x0d, 0);
> 
> No magic numbers?  Please define them.
> 
I will fix it in next patch-set.

> > +	unsigned int i;
> > +	int ret;
> > +
> > +	switch (chipset) {
> 
> Why a switch?  This would look better if you:
> 
> if (chipset != LPC_APL)
>    return -ENODEV;
> 
I will fix it in next patch-set.

> ... until you actually *need* a switch.
> 
> > +	case LPC_APL:
> > +		/*
> > +		 * Apollo lake, has not 1, but 4 gpio controllers,
> > +		 * handle it a bit differently.
> > +		 */
> > +
> > +		for (i = 0; i < ARRAY_SIZE(apl_gpio_io_res); i++) {
> > +			struct resource *res = apl_gpio_io_res[i];
> > +
> > +			apl_gpio_devices.resources = res;
> > +
> > +			/* Fill MEM resource */
> > +			ret = p2sb_bar(dev, apl_p2sb, res++);
> > +			if (ret)
> > +				goto warn_continue;
> 
> What does this do?
> 
The PCI devices acts strangely when enumerated by the kernel, including
rejecting all further PCI writes. As far as I know, it is also hidden for the sake
of Windows. We need to use Primary to Sideband bridge (p2sb) communication
interface in order to get IO or MMIO bar hidden by BIOS.

> > +			/* Fill IRQ resource */
> > +			res->start = APL_GPIO_IRQ;
> > +			res->end = res->start;
> > +			res->flags = IORESOURCE_IRQ;
> > +
> > +			apl_pinctrl_pdata.name = kasprintf(GFP_KERNEL,
> "%u",
> > +				i + 1);
> 
> All this to call a device "1", "2", etc?
> 
> > +			if (apl_pinctrl_pdata.name)
> > +				ret = mfd_add_devices(&dev->dev, i,
> > +					&apl_gpio_devices, 1, NULL, 0,
> NULL);
> 
> mfd_add_devices() is designed to take a group of devices and register them
> all for you.  Calling it once for each separate device you have is not correct.
> 
Rework the patches that look something like below:
	ret = mfd_add_devices(&dev->dev, apl_gpio_devices->id,
		apl_gpio_devices, ARRAY_SIZE(apl_gpio_devices),
			NULL, 0, NULL);

> > +			else
> > +				ret = -ENOMEM;
> > +
> > +warn_continue:
> > +			if (ret)
> > +				dev_warn(&dev->dev,
> > +					"Failed to add Apollo Lake GPIO %s:
> %d\n",
> > +						apl_pinctrl_pdata.name, ret);
> > +
> > +			kfree(apl_pinctrl_pdata.name);
> > +		}
> 
> This code just looks like one big hack to me.
> 
> Why don't you just declare the correct amount of resources in
> apl_gpio_devices instead of this hodge-podge?
> 
I will rewrite the patch following your comments that based on the correct
resources amount.

> > +		break;
> > +	default:
> > +		return -ENODEV;
> > +	}
> > +	return 0;
> > +}
> > +#else
> > +static inline int lpc_ich_misc(struct pci_dev *dev, enum lpc_chipsets
> > +chipset) {
> > +	return -ENODEV;
> > +}
> > +#endif /* CONFIG_X86_INTEL_NON_ACPI */
> 
> Don't do this in drivers.
> 
This is to solve kbuidbot complaint about kernel configuration, i.e.
CONFIG_PINCTRL=n. Appreciate if you could advise something on this.

> >  static int lpc_ich_probe(struct pci_dev *dev,
> >  				const struct pci_device_id *id)
> >  {
> > @@ -1093,6 +1218,9 @@ static int lpc_ich_probe(struct pci_dev *dev,
> >  			cell_added = true;
> >  	}
> >
> > +	if (!lpc_ich_misc(dev, priv->chipset))
> > +		cell_added = true;
> > +
> >  	/*
> >  	 * We only care if at least one or none of the cells registered
> >  	 * successfully.
> 
> --
> Lee Jones
> Linaro STMicroelectronics Landing Team Lead Linaro.org │ Open source
> software for ARM SoCs Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH v2 0/3] pinctrl/broxton: enable platform device in the absent of ACPI enumeration

[Andy Shevchenko]

On Tue, 2016-04-26 at 14:09 +0800, Tan Jui Nee wrote:
> Hi,
> The patches are to cater the need for non-ACPI system whereby
> a platform device has to be created in order to bind with
> Apollo Lake Pinctrl GPIO platform driver.
> 
> The MMIO BAR is accessed over the Primary to Sideband bridge
> (P2SB). Since the BIOS prevents the P2SB device from being
> enumerated by the PCI subsystem, so we need to hide/unhide P2SB
> to lookup the P2SB BAR and pass the PCI BAR address to the gpio
> platform driver.
> 
> All these three patches have dependencies on each other.
> 

Lee, Linus, I'm fine with the series, though I doubt which solution is
better to have the original driver independent as much as possible from
p2sb.

> Changes from V1:
> 	- Add new config option CONFIG_X86_INTEL_NON_ACPI and "select
> PINCTRL"
> 	  to fix kbuildbot error
> 
> Andy Shevchenko (1):
>   x86/platform/p2sb: New Primary to Sideband bridge support driver for
>     Intel SOC's
> 
> Tan Jui Nee (2):
>   pinctrl/broxton: enable platform device in the absent of ACPI
>     enumeration
>   mfd: lpc_ich: Add support for Intel Apollo Lake GPIO pinctrl in
>     non-ACPI system
> 
>  arch/x86/Kconfig                        |  14 ++++
>  arch/x86/include/asm/p2sb.h             |  27 +++++++
>  arch/x86/platform/intel/Makefile        |   1 +
>  arch/x86/platform/intel/p2sb.c          |  99
> ++++++++++++++++++++++++
>  drivers/mfd/Kconfig                     |   3 +-
>  drivers/mfd/lpc_ich.c                   | 128
> ++++++++++++++++++++++++++++++++
>  drivers/pinctrl/intel/pinctrl-broxton.c |  43 ++++++++---
>  7 files changed, 302 insertions(+), 13 deletions(-)
>  create mode 100644 arch/x86/include/asm/p2sb.h
>  create mode 100644 arch/x86/platform/intel/p2sb.c
> 

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html