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

[PATCH v6 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 in V6:
	- Rename CONFIG_X86_INTEL_APL to CONFIG_X86_INTEL_IVI so that it
	  relates to the actual product, as suggested by Mika.
	- Rework Makefile according Andy's comments.
	- Rename lpc_ich_misc() to lpc_ich_add_gpio() so that the name should not
	  be so generic, as suggested by Andy.
	- Call lpc_ich_add_gpio() via priv->chipset.
	- lpc_ich_add_gpio() function will be moved from 
	  .../include/linux/mfd/lpc_ich.h to
	  .../drivers/mfd/lpc_ich-apl.h
	  as this is a part of internal driver interface as suggested by Andy.
	- Move enum lpc_chipsets from 
	  .../drivers/mfd/lpc_ich-core.c to
	  .../include/linux/mfd/lpc_ich.h
	  as lpc_chipsets is also accessed by lpc_ich_add_gpio().
	- Check if kasprintf return value for all 4 gpio controllers before
	  proceed to add platform device by using mfd_add_devices().

Changes in V5:
	- Split lpc-ich driver into two parts (lpc_ich-core and lpc_ich-apl).
	  The file lpc_ich-apl.c introduces gpio platform driver in MFD.
	- Rename Kconfig option CONFIG_X86_INTEL_NON_ACPI to CONFIG_X86_INTEL_APL
	  so that it reflects actual product as suggested by Mika.
	- The patch: 
	  [PATCH] pinctrl/broxton: enable platform device in the absent of ACPI enumeration
	  is removed in V5 patch-set as the patch is already applied in Linus' pinctrl tree.

Changes in V4:
	- Move Kconfig option CONFIG_X86_INTEL_NON_ACPI from
	  [PATCH 2/3] x86/platform/p2sb: New Primary to Sideband bridge support driver for Intel SOC's
	  to
	  [PATCH 3/3] mfd: lpc_ich: Add support for Intel Apollo Lake GPIO pinctrl in non-ACPI system
	  since the config is used in latter patch.
	- Select CONFIG_P2SB when CONFIG_LPC_ICH is enabled.
	- Remove #ifdef CONFIG_X86_INTEL_NON_ACPI and use
	  #if defined(CONFIG_X86_INTEL_NON_ACPI) when lpc_ich_misc is called
	  as suggested by Lee Jones.
	- Use single dimensional array instead of 2D array for apl_gpio_io_res
	  structure and use DEFINE_RES_IRQ for its IRQ resource.

Changes in V3:
	- Simplify register addresses calculation and use DEFINE_RES_MEM_NAMED
	  defines for apl_gpio_io_res structure
	- Define magic number for P2SB PCI ID
	- Replace switch-case with if-else since currently we have only one
	  use case
	- Only call mfd_add_devices() once for all gpio communities

Changes in V2:
	- 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):
  mfd: lpc_ich: Rename lpc-ich driver
  mfd: lpc_ich: Add support for Intel Apollo Lake GPIO pinctrl in
    non-ACPI system

 arch/x86/Kconfig                          |  13 +++
 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/Makefile                      |   4 +
 drivers/mfd/lpc_ich-apl.c                 | 130 ++++++++++++++++++++++++++++++
 drivers/mfd/lpc_ich-apl.h                 |  28 +++++++
 drivers/mfd/{lpc_ich.c => lpc_ich-core.c} |  81 +++----------------
 include/linux/mfd/lpc_ich.h               |  73 +++++++++++++++++
 10 files changed, 387 insertions(+), 72 deletions(-)
 create mode 100644 arch/x86/include/asm/p2sb.h
 create mode 100644 arch/x86/platform/intel/p2sb.c
 create mode 100644 drivers/mfd/lpc_ich-apl.c
 create mode 100644 drivers/mfd/lpc_ich-apl.h
 rename drivers/mfd/{lpc_ich.c => lpc_ich-core.c} (94%)

-- 
1.9.1

[PATCH v6 1/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>
---
Changes in V6:
	- No change

Changes in V5:
	- No change

Changes in V4:
	- Move Kconfig option CONFIG_X86_INTEL_NON_ACPI from
	  [PATCH 2/3] x86/platform/p2sb: New Primary to Sideband bridge support driver for Intel SOC's
	  to
	  [PATCH 3/3] mfd: lpc_ich: Add support for Intel Apollo Lake GPIO pinctrl in non-ACPI system
	  since the config is used in latter patch.

Changes in V3:
	- No change

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

 arch/x86/Kconfig                 |  4 ++
 arch/x86/include/asm/p2sb.h      | 27 +++++++++++
 arch/x86/platform/intel/Makefile |  1 +
 arch/x86/platform/intel/p2sb.c   | 99 ++++++++++++++++++++++++++++++++++++++++
 4 files changed, 131 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 d9a94da..d305d81 100644
--- a/arch/x86/Kconfig
+++ b/arch/x86/Kconfig
@@ -604,6 +604,10 @@ config IOSF_MBI_DEBUG
 
 	  If you don't require the option or are in doubt, say N.
 
+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 v6 2/3] mfd: lpc_ich: Rename lpc-ich driver

[Tan Jui Nee]

This patch follows the example of mfd/wm831x to rename the driver
from "lpc_ich" to "lpc_ich-core".

Signed-off-by: Tan Jui Nee <jui.nee.tan@intel.com>
---
Changes in V6:
	- none, just a subject line and commit message change.

 drivers/mfd/Makefile                      | 1 +
 drivers/mfd/{lpc_ich.c => lpc_ich-core.c} | 0
 2 files changed, 1 insertion(+)
 rename drivers/mfd/{lpc_ich.c => lpc_ich-core.c} (100%)

diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
index 42a66e1..1dfe5fb 100644
--- a/drivers/mfd/Makefile
+++ b/drivers/mfd/Makefile
@@ -155,6 +155,7 @@ obj-$(CONFIG_PMIC_ADP5520)	+= adp5520.o
 obj-$(CONFIG_MFD_KEMPLD)	+= kempld-core.o
 obj-$(CONFIG_MFD_INTEL_QUARK_I2C_GPIO)	+= intel_quark_i2c_gpio.o
 obj-$(CONFIG_LPC_SCH)		+= lpc_sch.o
+lpc_ich-objs		:= lpc_ich-core.o
 obj-$(CONFIG_LPC_ICH)		+= lpc_ich.o
 obj-$(CONFIG_MFD_RDC321X)	+= rdc321x-southbridge.o
 obj-$(CONFIG_MFD_JANZ_CMODIO)	+= janz-cmodio.o
diff --git a/drivers/mfd/lpc_ich.c b/drivers/mfd/lpc_ich-core.c
similarity index 100%
rename from drivers/mfd/lpc_ich.c
rename to drivers/mfd/lpc_ich-core.c
-- 
1.9.1

[PATCH v6 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>
---
Changes in V6:
	- Rename CONFIG_X86_INTEL_APL to CONFIG_X86_INTEL_IVI so that it
	  relates to the actual product, as suggested by Mika.
	- Rework Makefile according Andy's comments.
	- Rename lpc_ich_misc() to lpc_ich_add_gpio() so that the name should not
	  be so generic, as suggested by Andy.
	- Call lpc_ich_add_gpio() via priv->chipset.
	- lpc_ich_add_gpio() function will be moved from 
	  .../include/linux/mfd/lpc_ich.h to
	  .../drivers/mfd/lpc_ich-apl.h
	  as this is a part of internal driver interface as suggested by Andy.
	- Move enum lpc_chipsets from 
	  .../drivers/mfd/lpc_ich-core.c to
	  .../include/linux/mfd/lpc_ich.h
	  as lpc_chipsets is also accessed by lpc_ich_add_gpio().
	- Check if kasprintf return value for all 4 gpio controllers before
	  proceed to add platform device by using mfd_add_devices().

Changes in V5:
	- Split lpc-ich driver into two parts (lpc_ich-core and lpc_ich-apl).
	  The file lpc_ich-apl.c introduces gpio platform driver in MFD.
	- Rename Kconfig option CONFIG_X86_INTEL_NON_ACPI to CONFIG_X86_INTEL_APL
	  so that it reflects actual product as suggested by Mika.

Changes in V4:
	- Move Kconfig option CONFIG_X86_INTEL_NON_ACPI from
	  [PATCH 2/3] x86/platform/p2sb: New Primary to Sideband bridge support driver for Intel SOC's
	  to
	  [PATCH 3/3] mfd: lpc_ich: Add support for Intel Apollo Lake GPIO pinctrl in non-ACPI system
	  since the config is used in latter patch.
	- Select CONFIG_P2SB when CONFIG_LPC_ICH is enabled.
	- Remove #ifdef CONFIG_X86_INTEL_NON_ACPI and use
	  #if defined(CONFIG_X86_INTEL_NON_ACPI) when lpc_ich_misc is called
	  as suggested by Lee Jones.
	- Use single dimensional array instead of 2D array for apl_gpio_io_res
	  structure and use DEFINE_RES_IRQ for its IRQ resource.

Changes in V3:
	- Simplify register addresses calculation and use DEFINE_RES_MEM_NAMED
	  defines for apl_gpio_io_res structure
	- Define magic number for P2SB PCI ID
	- Replace switch-case with if-else since currently we have only one
	  use case
	- Only call mfd_add_devices() once for all gpio communities

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

 arch/x86/Kconfig            |   9 +++
 drivers/mfd/Kconfig         |   3 +-
 drivers/mfd/Makefile        |   5 +-
 drivers/mfd/lpc_ich-apl.c   | 130 ++++++++++++++++++++++++++++++++++++++++++++
 drivers/mfd/lpc_ich-apl.h   |  28 ++++++++++
 drivers/mfd/lpc_ich-core.c  |  81 ++++-----------------------
 include/linux/mfd/lpc_ich.h |  73 +++++++++++++++++++++++++
 7 files changed, 256 insertions(+), 73 deletions(-)
 create mode 100644 drivers/mfd/lpc_ich-apl.c
 create mode 100644 drivers/mfd/lpc_ich-apl.h

diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
index d305d81..c0b427b 100644
--- a/arch/x86/Kconfig
+++ b/arch/x86/Kconfig
@@ -513,6 +513,15 @@ config X86_INTEL_CE
 	  This option compiles in support for the CE4100 SOC for settop
 	  boxes and media devices.
 
+config X86_INTEL_IVI
+	bool "Intel in-vehicle infotainment (IVI) systems used in cars"
+	select PINCTRL
+	---help---
+	  Select this option to enable MMIO BAR access over the P2SB for
+	  non-ACPI Intel Apollo Lake SoC platforms. This driver uses the P2SB
+	  hide/unhide mechanism cooperatively to pass the PCI BAR address to
+	  the platform driver, currently GPIO.
+
 config X86_INTEL_MID
 	bool "Intel MID platform support"
 	depends on X86_EXTENDED_PLATFORM
diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
index 1bcf601..dc4e543 100644
--- a/drivers/mfd/Kconfig
+++ b/drivers/mfd/Kconfig
@@ -369,8 +369,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
 	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/Makefile b/drivers/mfd/Makefile
index 1dfe5fb..0aa3e1f 100644
--- a/drivers/mfd/Makefile
+++ b/drivers/mfd/Makefile
@@ -155,8 +155,11 @@ obj-$(CONFIG_PMIC_ADP5520)	+= adp5520.o
 obj-$(CONFIG_MFD_KEMPLD)	+= kempld-core.o
 obj-$(CONFIG_MFD_INTEL_QUARK_I2C_GPIO)	+= intel_quark_i2c_gpio.o
 obj-$(CONFIG_LPC_SCH)		+= lpc_sch.o
-lpc_ich-objs		:= lpc_ich-core.o
 obj-$(CONFIG_LPC_ICH)		+= lpc_ich.o
+lpc_ich-objs		:= lpc_ich-core.o
+ifeq ($(CONFIG_X86_INTEL_IVI),y)
+lpc_ich-objs += lpc_ich-apl.o
+endif
 obj-$(CONFIG_MFD_RDC321X)	+= rdc321x-southbridge.o
 obj-$(CONFIG_MFD_JANZ_CMODIO)	+= janz-cmodio.o
 obj-$(CONFIG_MFD_JZ4740_ADC)	+= jz4740-adc.o
diff --git a/drivers/mfd/lpc_ich-apl.c b/drivers/mfd/lpc_ich-apl.c
new file mode 100644
index 0000000..e4b9688
--- /dev/null
+++ b/drivers/mfd/lpc_ich-apl.c
@@ -0,0 +1,130 @@
+/*
+ * Purpose: Create a platform device to bind with Intel Apollo Lake
+ * Pinctrl GPIO platform driver
+ * Copyright (C) 2016 Intel Corporation
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#include <linux/pci.h>
+#include <linux/mfd/core.h>
+#include <linux/mfd/lpc_ich.h>
+#include <linux/pinctrl/pinctrl.h>
+#include <asm/p2sb.h>
+
+#include "lpc_ich-apl.h"
+
+/* Offset data for Apollo Lake GPIO communities */
+#define APL_GPIO_SOUTHWEST_OFFSET	0xc00000
+#define APL_GPIO_NORTHWEST_OFFSET	0xc40000
+#define APL_GPIO_NORTH_OFFSET		0xc50000
+#define APL_GPIO_WEST_OFFSET		0xc70000
+
+#define APL_GPIO_SOUTHWEST_NPIN		43
+#define APL_GPIO_NORTHWEST_NPIN		77
+#define APL_GPIO_NORTH_NPIN		78
+#define APL_GPIO_WEST_NPIN		47
+
+#define APL_GPIO_IRQ 14
+
+#define PCI_IDSEL_P2SB	0x0d
+
+static struct resource apl_gpio_io_res[] = {
+	DEFINE_RES_MEM_NAMED(APL_GPIO_NORTH_OFFSET,
+		APL_GPIO_NORTH_NPIN * SZ_8, "apl_pinctrl_n"),
+	DEFINE_RES_MEM_NAMED(APL_GPIO_NORTHWEST_OFFSET,
+		APL_GPIO_NORTHWEST_NPIN * SZ_8, "apl_pinctrl_nw"),
+	DEFINE_RES_MEM_NAMED(APL_GPIO_WEST_OFFSET,
+		APL_GPIO_WEST_NPIN * SZ_8, "apl_pinctrl_w"),
+	DEFINE_RES_MEM_NAMED(APL_GPIO_SOUTHWEST_OFFSET,
+		APL_GPIO_SOUTHWEST_NPIN * SZ_8, "apl_pinctrl_sw"),
+	DEFINE_RES_IRQ(APL_GPIO_IRQ),
+};
+
+static struct pinctrl_pin_desc apl_pinctrl_pdata;
+
+static struct mfd_cell apl_gpio_devices[] = {
+	{
+		.name = "apl-pinctrl",
+		.id = 0,
+		.num_resources = ARRAY_SIZE(apl_gpio_io_res),
+		.resources = apl_gpio_io_res,
+		.pdata_size = sizeof(apl_pinctrl_pdata),
+		.platform_data = &apl_pinctrl_pdata,
+		.ignore_resource_conflicts = true,
+	},
+	{
+		.name = "apl-pinctrl",
+		.id = 1,
+		.num_resources = ARRAY_SIZE(apl_gpio_io_res),
+		.resources = apl_gpio_io_res,
+		.pdata_size = sizeof(apl_pinctrl_pdata),
+		.platform_data = &apl_pinctrl_pdata,
+		.ignore_resource_conflicts = true,
+	},
+	{
+		.name = "apl-pinctrl",
+		.id = 2,
+		.num_resources = ARRAY_SIZE(apl_gpio_io_res),
+		.resources = apl_gpio_io_res,
+		.pdata_size = sizeof(apl_pinctrl_pdata),
+		.platform_data = &apl_pinctrl_pdata,
+		.ignore_resource_conflicts = true,
+	},
+	{
+		.name = "apl-pinctrl",
+		.id = 3,
+		.num_resources = ARRAY_SIZE(apl_gpio_io_res),
+		.resources = apl_gpio_io_res,
+		.pdata_size = sizeof(apl_pinctrl_pdata),
+		.platform_data = &apl_pinctrl_pdata,
+		.ignore_resource_conflicts = true,
+	},
+};
+
+int lpc_ich_add_gpio(struct pci_dev *dev, enum lpc_chipsets chipset)
+{
+	unsigned int apl_p2sb = PCI_DEVFN(PCI_IDSEL_P2SB, 0);
+	unsigned int i;
+	int ret;
+
+	if (chipset != LPC_APL)
+		return -ENODEV;
+	/*
+	 * Apollo lake, has not 1, but 4 gpio controllers,
+	 * handle it a bit differently.
+	 */
+
+	for (i = 0; i < ARRAY_SIZE(apl_gpio_io_res)-1; i++) {
+		struct resource *res = &apl_gpio_io_res[i];
+
+		apl_gpio_devices[i].resources = res;
+
+		/* Fill MEM resource */
+		ret = p2sb_bar(dev, apl_p2sb, res++);
+		if (ret)
+			goto warn_continue;
+
+		apl_pinctrl_pdata.name = kasprintf(GFP_KERNEL, "%u",
+			i + 1);
+		if (apl_pinctrl_pdata.name == NULL) {
+			ret = -ENOMEM;
+			goto warn_continue;
+		}
+	}
+
+	ret = mfd_add_devices(&dev->dev, 0,
+		apl_gpio_devices, ARRAY_SIZE(apl_gpio_devices),
+			NULL, 0, NULL);
+
+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);
+	return 0;
+}
diff --git a/drivers/mfd/lpc_ich-apl.h b/drivers/mfd/lpc_ich-apl.h
new file mode 100644
index 0000000..db8325d
--- /dev/null
+++ b/drivers/mfd/lpc_ich-apl.h
@@ -0,0 +1,28 @@
+/*
+ * lpc_ich-apl.h - Intel in-vehicle infotainment (IVI) systems used in cars
+ *                 support
+ *
+ * Copyright (C) 2016, Intel Corporation
+ *
+ * Authors: Tan, Jui Nee <jui.nee.tan@intel.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#ifndef __LPC_ICH_APL_H__
+#define __LPC_ICH_APL_H__
+
+#include <linux/pci.h>
+
+#if IS_ENABLED(CONFIG_X86_INTEL_IVI)
+int lpc_ich_add_gpio(struct pci_dev *dev, enum lpc_chipsets chipset);
+#else /* CONFIG_X86_INTEL_IVI is not set */
+static inline int lpc_ich_add_gpio(struct pci_dev *dev, enum lpc_chipsets chipset)
+{
+	return -ENODEV;
+}
+#endif
+
+#endif
diff --git a/drivers/mfd/lpc_ich-core.c b/drivers/mfd/lpc_ich-core.c
index bd3aa45..e09d1e2 100644
--- a/drivers/mfd/lpc_ich-core.c
+++ b/drivers/mfd/lpc_ich-core.c
@@ -69,6 +69,8 @@
 #include <linux/mfd/lpc_ich.h>
 #include <linux/platform_data/itco_wdt.h>
 
+#include "lpc_ich-apl.h"
+
 #define ACPIBASE		0x40
 #define ACPIBASE_GPE_OFF	0x28
 #define ACPIBASE_GPE_END	0x2f
@@ -147,77 +149,6 @@ static struct mfd_cell lpc_ich_gpio_cell = {
 	.ignore_resource_conflicts = true,
 };
 
-/* chipset related info */
-enum lpc_chipsets {
-	LPC_ICH = 0,	/* ICH */
-	LPC_ICH0,	/* ICH0 */
-	LPC_ICH2,	/* ICH2 */
-	LPC_ICH2M,	/* ICH2-M */
-	LPC_ICH3,	/* ICH3-S */
-	LPC_ICH3M,	/* ICH3-M */
-	LPC_ICH4,	/* ICH4 */
-	LPC_ICH4M,	/* ICH4-M */
-	LPC_CICH,	/* C-ICH */
-	LPC_ICH5,	/* ICH5 & ICH5R */
-	LPC_6300ESB,	/* 6300ESB */
-	LPC_ICH6,	/* ICH6 & ICH6R */
-	LPC_ICH6M,	/* ICH6-M */
-	LPC_ICH6W,	/* ICH6W & ICH6RW */
-	LPC_631XESB,	/* 631xESB/632xESB */
-	LPC_ICH7,	/* ICH7 & ICH7R */
-	LPC_ICH7DH,	/* ICH7DH */
-	LPC_ICH7M,	/* ICH7-M & ICH7-U */
-	LPC_ICH7MDH,	/* ICH7-M DH */
-	LPC_NM10,	/* NM10 */
-	LPC_ICH8,	/* ICH8 & ICH8R */
-	LPC_ICH8DH,	/* ICH8DH */
-	LPC_ICH8DO,	/* ICH8DO */
-	LPC_ICH8M,	/* ICH8M */
-	LPC_ICH8ME,	/* ICH8M-E */
-	LPC_ICH9,	/* ICH9 */
-	LPC_ICH9R,	/* ICH9R */
-	LPC_ICH9DH,	/* ICH9DH */
-	LPC_ICH9DO,	/* ICH9DO */
-	LPC_ICH9M,	/* ICH9M */
-	LPC_ICH9ME,	/* ICH9M-E */
-	LPC_ICH10,	/* ICH10 */
-	LPC_ICH10R,	/* ICH10R */
-	LPC_ICH10D,	/* ICH10D */
-	LPC_ICH10DO,	/* ICH10DO */
-	LPC_PCH,	/* PCH Desktop Full Featured */
-	LPC_PCHM,	/* PCH Mobile Full Featured */
-	LPC_P55,	/* P55 */
-	LPC_PM55,	/* PM55 */
-	LPC_H55,	/* H55 */
-	LPC_QM57,	/* QM57 */
-	LPC_H57,	/* H57 */
-	LPC_HM55,	/* HM55 */
-	LPC_Q57,	/* Q57 */
-	LPC_HM57,	/* HM57 */
-	LPC_PCHMSFF,	/* PCH Mobile SFF Full Featured */
-	LPC_QS57,	/* QS57 */
-	LPC_3400,	/* 3400 */
-	LPC_3420,	/* 3420 */
-	LPC_3450,	/* 3450 */
-	LPC_EP80579,	/* EP80579 */
-	LPC_CPT,	/* Cougar Point */
-	LPC_CPTD,	/* Cougar Point Desktop */
-	LPC_CPTM,	/* Cougar Point Mobile */
-	LPC_PBG,	/* Patsburg */
-	LPC_DH89XXCC,	/* DH89xxCC */
-	LPC_PPT,	/* Panther Point */
-	LPC_LPT,	/* Lynx Point */
-	LPC_LPT_LP,	/* Lynx Point-LP */
-	LPC_WBG,	/* Wellsburg */
-	LPC_AVN,	/* Avoton SoC */
-	LPC_BAYTRAIL,   /* Bay Trail SoC */
-	LPC_COLETO,	/* Coleto Creek */
-	LPC_WPT_LP,	/* Wildcat Point-LP */
-	LPC_BRASWELL,	/* Braswell SoC */
-	LPC_LEWISBURG,	/* Lewisburg */
-	LPC_9S,		/* 9 Series */
-};
-
 static struct lpc_ich_info lpc_chipset_info[] = {
 	[LPC_ICH] = {
 		.name = "ICH",
@@ -531,6 +462,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 +614,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},
@@ -1093,6 +1029,9 @@ static int lpc_ich_probe(struct pci_dev *dev,
 			cell_added = true;
 	}
 
+	if (!lpc_ich_add_gpio(dev, priv->chipset))
+		cell_added = true;
+
 	/*
 	 * We only care if at least one or none of the cells registered
 	 * successfully.
diff --git a/include/linux/mfd/lpc_ich.h b/include/linux/mfd/lpc_ich.h
index 2b300b4..03c2ca3 100644
--- a/include/linux/mfd/lpc_ich.h
+++ b/include/linux/mfd/lpc_ich.h
@@ -34,6 +34,7 @@ enum {
 	ICH_V10CORP_GPIO,
 	ICH_V10CONS_GPIO,
 	AVOTON_GPIO,
+	APL_GPIO,
 };
 
 struct lpc_ich_info {
@@ -43,4 +44,76 @@ struct lpc_ich_info {
 	u8 use_gpio;
 };
 
+/* chipset related info */
+enum lpc_chipsets {
+	LPC_ICH = 0,	/* ICH */
+	LPC_ICH0,	/* ICH0 */
+	LPC_ICH2,	/* ICH2 */
+	LPC_ICH2M,	/* ICH2-M */
+	LPC_ICH3,	/* ICH3-S */
+	LPC_ICH3M,	/* ICH3-M */
+	LPC_ICH4,	/* ICH4 */
+	LPC_ICH4M,	/* ICH4-M */
+	LPC_CICH,	/* C-ICH */
+	LPC_ICH5,	/* ICH5 & ICH5R */
+	LPC_6300ESB,	/* 6300ESB */
+	LPC_ICH6,	/* ICH6 & ICH6R */
+	LPC_ICH6M,	/* ICH6-M */
+	LPC_ICH6W,	/* ICH6W & ICH6RW */
+	LPC_631XESB,	/* 631xESB/632xESB */
+	LPC_ICH7,	/* ICH7 & ICH7R */
+	LPC_ICH7DH,	/* ICH7DH */
+	LPC_ICH7M,	/* ICH7-M & ICH7-U */
+	LPC_ICH7MDH,	/* ICH7-M DH */
+	LPC_NM10,	/* NM10 */
+	LPC_ICH8,	/* ICH8 & ICH8R */
+	LPC_ICH8DH,	/* ICH8DH */
+	LPC_ICH8DO,	/* ICH8DO */
+	LPC_ICH8M,	/* ICH8M */
+	LPC_ICH8ME,	/* ICH8M-E */
+	LPC_ICH9,	/* ICH9 */
+	LPC_ICH9R,	/* ICH9R */
+	LPC_ICH9DH,	/* ICH9DH */
+	LPC_ICH9DO,	/* ICH9DO */
+	LPC_ICH9M,	/* ICH9M */
+	LPC_ICH9ME,	/* ICH9M-E */
+	LPC_ICH10,	/* ICH10 */
+	LPC_ICH10R,	/* ICH10R */
+	LPC_ICH10D,	/* ICH10D */
+	LPC_ICH10DO,	/* ICH10DO */
+	LPC_PCH,	/* PCH Desktop Full Featured */
+	LPC_PCHM,	/* PCH Mobile Full Featured */
+	LPC_P55,	/* P55 */
+	LPC_PM55,	/* PM55 */
+	LPC_H55,	/* H55 */
+	LPC_QM57,	/* QM57 */
+	LPC_H57,	/* H57 */
+	LPC_HM55,	/* HM55 */
+	LPC_Q57,	/* Q57 */
+	LPC_HM57,	/* HM57 */
+	LPC_PCHMSFF,	/* PCH Mobile SFF Full Featured */
+	LPC_QS57,	/* QS57 */
+	LPC_3400,	/* 3400 */
+	LPC_3420,	/* 3420 */
+	LPC_3450,	/* 3450 */
+	LPC_EP80579,	/* EP80579 */
+	LPC_CPT,	/* Cougar Point */
+	LPC_CPTD,	/* Cougar Point Desktop */
+	LPC_CPTM,	/* Cougar Point Mobile */
+	LPC_PBG,	/* Patsburg */
+	LPC_DH89XXCC,	/* DH89xxCC */
+	LPC_PPT,	/* Panther Point */
+	LPC_LPT,	/* Lynx Point */
+	LPC_LPT_LP,	/* Lynx Point-LP */
+	LPC_WBG,	/* Wellsburg */
+	LPC_AVN,	/* Avoton SoC */
+	LPC_BAYTRAIL,   /* Bay Trail SoC */
+	LPC_COLETO,	/* Coleto Creek */
+	LPC_WPT_LP,	/* Wildcat Point-LP */
+	LPC_BRASWELL,	/* Braswell SoC */
+	LPC_LEWISBURG,	/* Lewisburg */
+	LPC_9S,		/* 9 Series */
+	LPC_APL,	/* Apollo Lake SoC */
+};
+
 #endif
-- 
1.9.1

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

[Paul Gortmaker]

On Thu, Jul 14, 2016 at 4:11 AM, Tan Jui Nee <jui.nee.tan@intel.com> wrote:
> 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>
> ---
> Changes in V6:
>         - No change
>
> Changes in V5:
>         - No change
>
> Changes in V4:
>         - Move Kconfig option CONFIG_X86_INTEL_NON_ACPI from
>           [PATCH 2/3] x86/platform/p2sb: New Primary to Sideband bridge support driver for Intel SOC's
>           to
>           [PATCH 3/3] mfd: lpc_ich: Add support for Intel Apollo Lake GPIO pinctrl in non-ACPI system
>           since the config is used in latter patch.
>
> Changes in V3:
>         - No change
>
> Changes in V2:
>         - Add new config option CONFIG_X86_INTEL_NON_ACPI and "select PINCTRL"
>           to fix kbuildbot error
>
>  arch/x86/Kconfig                 |  4 ++
>  arch/x86/include/asm/p2sb.h      | 27 +++++++++++
>  arch/x86/platform/intel/Makefile |  1 +
>  arch/x86/platform/intel/p2sb.c   | 99 ++++++++++++++++++++++++++++++++++++++++
>  4 files changed, 131 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 d9a94da..d305d81 100644
> --- a/arch/x86/Kconfig
> +++ b/arch/x86/Kconfig
> @@ -604,6 +604,10 @@ config IOSF_MBI_DEBUG
>
>           If you don't require the option or are in doubt, say N.
>
> +config P2SB
> +       tristate

OK, this is tristate, but then....

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

...and module.h is included, but yet...

> +#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");

...the above is the only modular "use" that I can find.  So is the
tristate bogus?   Without a module_init and a module_exit I am
confused....

I just finished an audit of arch/x86 for bogus uses of module.h
so I'd like to ensure we don't add more.

Thanks,
Paul.
--

> --
> 1.9.1
>

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

[Tan, Jui Nee]

> -----Original Message-----
> From: paul.gortmaker@gmail.com [mailto:paul.gortmaker@gmail.com] On
> Behalf Of Paul Gortmaker
> Sent: Friday, July 15, 2016 8:01 AM
> 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; H. Peter Anvin <hpa@zytor.com>; X86 ML
> <x86@kernel.org>; ptyser@xes-inc.com; Lee Jones <lee.jones@linaro.org>;
> Linus Walleij <linus.walleij@linaro.org>; linux-gpio@vger.kernel.org; LKML
> <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 v6 1/3] x86/platform/p2sb: New Primary to Sideband
> bridge support driver for Intel SOC's
> 
> On Thu, Jul 14, 2016 at 4:11 AM, Tan Jui Nee <jui.nee.tan@intel.com> wrote:
> > 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>
> > ---
> > Changes in V6:
> >         - No change
> >
> > Changes in V5:
> >         - No change
> >
> > Changes in V4:
> >         - Move Kconfig option CONFIG_X86_INTEL_NON_ACPI from
> >           [PATCH 2/3] x86/platform/p2sb: New Primary to Sideband bridge
> support driver for Intel SOC's
> >           to
> >           [PATCH 3/3] mfd: lpc_ich: Add support for Intel Apollo Lake GPIO
> pinctrl in non-ACPI system
> >           since the config is used in latter patch.
> >
> > Changes in V3:
> >         - No change
> >
> > Changes in V2:
> >         - Add new config option CONFIG_X86_INTEL_NON_ACPI and "select
> PINCTRL"
> >           to fix kbuildbot error
> >
> >  arch/x86/Kconfig                 |  4 ++
> >  arch/x86/include/asm/p2sb.h      | 27 +++++++++++
> >  arch/x86/platform/intel/Makefile |  1 +
> >  arch/x86/platform/intel/p2sb.c   | 99
> ++++++++++++++++++++++++++++++++++++++++
> >  4 files changed, 131 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
> > d9a94da..d305d81 100644
> > --- a/arch/x86/Kconfig
> > +++ b/arch/x86/Kconfig
> > @@ -604,6 +604,10 @@ config IOSF_MBI_DEBUG
> >
> >           If you don't require the option or are in doubt, say N.
> >
> > +config P2SB
> > +       tristate
> 
> OK, this is tristate, but then....
> 
P2SB is tristate as currently it is only used by LPC_ICH that is tristate too.
...
config LPC_ICH
	tristate "Intel ICH LPC"
	depends on X86 && PCI
	select MFD_CORE
	select P2SB
...
> > +       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>
> 
> ...and module.h is included, but yet...
> 
> > +#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");
> 
> ...the above is the only modular "use" that I can find.  So is the
> tristate bogus?   Without a module_init and a module_exit I am
> confused....
> 
> I just finished an audit of arch/x86 for bogus uses of module.h so I'd like to
> ensure we don't add more.
> 
> Thanks,
> Paul.
> --
> 
P2SB could be "bool" instead of tristate. 
My concern is if LPC_ICH built as module and not loaded, P2SB might
consume memory when P2SB is not being used.
What do you think? If that's ok for you, my next patch will be something like
this:
...
config P2SB
	bool
	depends on PCI
...
In p2sb.c, module.h header file will be removed as well.
Hi Andy, please provide your comments and/or concerns if any. Thanks.
> > --
> > 1.9.1
> >

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

[Tan, Jui Nee]

> -----Original Message-----
> From: paul.gortmaker@gmail.com [mailto:paul.gortmaker@gmail.com] On
> Behalf Of Paul Gortmaker
> Sent: Friday, July 15, 2016 8:01 AM
> 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; H. Peter Anvin <hpa@zytor.com>; X86 ML
> <x86@kernel.org>; ptyser@xes-inc.com; Lee Jones <lee.jones@linaro.org>;
> Linus Walleij <linus.walleij@linaro.org>; linux-gpio@vger.kernel.org; LKML
> <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 v6 1/3] x86/platform/p2sb: New Primary to Sideband
> bridge support driver for Intel SOC's
> 
> On Thu, Jul 14, 2016 at 4:11 AM, Tan Jui Nee <jui.nee.tan@intel.com> wrote:
> > 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>
> > ---
> > Changes in V6:
> >         - No change
> >
> > Changes in V5:
> >         - No change
> >
> > Changes in V4:
> >         - Move Kconfig option CONFIG_X86_INTEL_NON_ACPI from
> >           [PATCH 2/3] x86/platform/p2sb: New Primary to Sideband bridge
> support driver for Intel SOC's
> >           to
> >           [PATCH 3/3] mfd: lpc_ich: Add support for Intel Apollo Lake GPIO
> pinctrl in non-ACPI system
> >           since the config is used in latter patch.
> >
> > Changes in V3:
> >         - No change
> >
> > Changes in V2:
> >         - Add new config option CONFIG_X86_INTEL_NON_ACPI and "select
> PINCTRL"
> >           to fix kbuildbot error
> >
> >  arch/x86/Kconfig                 |  4 ++
> >  arch/x86/include/asm/p2sb.h      | 27 +++++++++++
> >  arch/x86/platform/intel/Makefile |  1 +
> >  arch/x86/platform/intel/p2sb.c   | 99
> ++++++++++++++++++++++++++++++++++++++++
> >  4 files changed, 131 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
> > d9a94da..d305d81 100644
> > --- a/arch/x86/Kconfig
> > +++ b/arch/x86/Kconfig
> > @@ -604,6 +604,10 @@ config IOSF_MBI_DEBUG
> >
> >           If you don't require the option or are in doubt, say N.
> >
> > +config P2SB
> > +       tristate
> 
> OK, this is tristate, but then....
> 
P2SB is tristate as currently it is only used by LPC_ICH that is tristate too.
...
config LPC_ICH
	tristate "Intel ICH LPC"
	depends on X86 && PCI
	select MFD_CORE
	select P2SB
...
> > +       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>
> 
> ...and module.h is included, but yet...
> 
> > +#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");
> 
> ...the above is the only modular "use" that I can find.  So is the
> tristate bogus?   Without a module_init and a module_exit I am
> confused....
> 
> I just finished an audit of arch/x86 for bogus uses of module.h so I'd like to
> ensure we don't add more.
> 
> Thanks,
> Paul.
> --
> 
P2SB could be "bool" instead of tristate. 
My concern is if LPC_ICH built as module and not loaded, P2SB might
consume memory when P2SB is not being used.
What do you think? If that's ok for you, my next patch will be something like
this:
...
config P2SB
	bool
	depends on PCI
...
In p2sb.c, module.h header file will be removed as well.
Hi Andy, please provide your comments and/or concerns if any. Thanks.
> > --
> > 1.9.1
> >

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

[Tan, Jui Nee]

> -----Original Message-----
> From: Tan, Jui Nee
> Sent: Monday, July 18, 2016 11:35 AM
> To: 'Paul Gortmaker' <paul.gortmaker@windriver.com>;
> andriy.shevchenko@linux.intel.com
> Cc: mika.westerberg@linux.intel.com; heikki.krogerus@linux.intel.com;
> tglx@linutronix.de; mingo@redhat.com; H. Peter Anvin <hpa@zytor.com>;
> X86 ML <x86@kernel.org>; ptyser@xes-inc.com; Lee Jones
> <lee.jones@linaro.org>; Linus Walleij <linus.walleij@linaro.org>; linux-
> gpio@vger.kernel.org; LKML <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 v6 1/3] x86/platform/p2sb: New Primary to Sideband
> bridge support driver for Intel SOC's
> 
> 
> 
> > -----Original Message-----
> > From: paul.gortmaker@gmail.com [mailto:paul.gortmaker@gmail.com] On
> > Behalf Of Paul Gortmaker
> > Sent: Friday, July 15, 2016 8:01 AM
> > 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; H. Peter Anvin <hpa@zytor.com>; X86 ML
> > <x86@kernel.org>; ptyser@xes-inc.com; Lee Jones
> <lee.jones@linaro.org>;
> > Linus Walleij <linus.walleij@linaro.org>; linux-gpio@vger.kernel.org; LKML
> > <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 v6 1/3] x86/platform/p2sb: New Primary to Sideband
> > bridge support driver for Intel SOC's
> >
> > On Thu, Jul 14, 2016 at 4:11 AM, Tan Jui Nee <jui.nee.tan@intel.com>
> wrote:
> > > 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>
> > > ---
> > > Changes in V6:
> > >         - No change
> > >
> > > Changes in V5:
> > >         - No change
> > >
> > > Changes in V4:
> > >         - Move Kconfig option CONFIG_X86_INTEL_NON_ACPI from
> > >           [PATCH 2/3] x86/platform/p2sb: New Primary to Sideband bridge
> > support driver for Intel SOC's
> > >           to
> > >           [PATCH 3/3] mfd: lpc_ich: Add support for Intel Apollo Lake GPIO
> > pinctrl in non-ACPI system
> > >           since the config is used in latter patch.
> > >
> > > Changes in V3:
> > >         - No change
> > >
> > > Changes in V2:
> > >         - Add new config option CONFIG_X86_INTEL_NON_ACPI and "select
> > PINCTRL"
> > >           to fix kbuildbot error
> > >
> > >  arch/x86/Kconfig                 |  4 ++
> > >  arch/x86/include/asm/p2sb.h      | 27 +++++++++++
> > >  arch/x86/platform/intel/Makefile |  1 +
> > >  arch/x86/platform/intel/p2sb.c   | 99
> > ++++++++++++++++++++++++++++++++++++++++
> > >  4 files changed, 131 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
> > > d9a94da..d305d81 100644
> > > --- a/arch/x86/Kconfig
> > > +++ b/arch/x86/Kconfig
> > > @@ -604,6 +604,10 @@ config IOSF_MBI_DEBUG
> > >
> > >           If you don't require the option or are in doubt, say N.
> > >
> > > +config P2SB
> > > +       tristate
> >
> > OK, this is tristate, but then....
> >
> P2SB is tristate as currently it is only used by LPC_ICH that is tristate too.
> ...
> config LPC_ICH
> 	tristate "Intel ICH LPC"
> 	depends on X86 && PCI
> 	select MFD_CORE
> 	select P2SB
> ...
> > > +       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>
> >
> > ...and module.h is included, but yet...
> >
> > > +#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");
> >
> > ...the above is the only modular "use" that I can find.  So is the
> > tristate bogus?   Without a module_init and a module_exit I am
> > confused....
> >
> > I just finished an audit of arch/x86 for bogus uses of module.h so I'd like to
> > ensure we don't add more.
> >
> > Thanks,
> > Paul.
> > --
> >
> P2SB could be "bool" instead of tristate.
> My concern is if LPC_ICH built as module and not loaded, P2SB might
> consume memory when P2SB is not being used.
What I meant to say is, when LPC_ICH is built but not use by the current
hardware, the P2SB compiled code will occupy memory and cannot be
unloaded even if it is not in use. If memory usage is not the concern, I will
rework this patch to use Boolean option.
> What do you think? If that's ok for you, my next patch will be something like
> this:
> ...
> config P2SB
> 	bool
> 	depends on PCI
> ...
> In p2sb.c, module.h header file will be removed as well.
> Hi Andy, please provide your comments and/or concerns if any. Thanks.
> > > --
> > > 1.9.1
> > >

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

[Tan, Jui Nee]

> -----Original Message-----
> From: Tan, Jui Nee
> Sent: Monday, July 18, 2016 11:35 AM
> To: 'Paul Gortmaker' <paul.gortmaker@windriver.com>;
> andriy.shevchenko@linux.intel.com
> Cc: mika.westerberg@linux.intel.com; heikki.krogerus@linux.intel.com;
> tglx@linutronix.de; mingo@redhat.com; H. Peter Anvin <hpa@zytor.com>;
> X86 ML <x86@kernel.org>; ptyser@xes-inc.com; Lee Jones
> <lee.jones@linaro.org>; Linus Walleij <linus.walleij@linaro.org>; linux-
> gpio@vger.kernel.org; LKML <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 v6 1/3] x86/platform/p2sb: New Primary to Sideband
> bridge support driver for Intel SOC's
> 
> 
> 
> > -----Original Message-----
> > From: paul.gortmaker@gmail.com [mailto:paul.gortmaker@gmail.com] On
> > Behalf Of Paul Gortmaker
> > Sent: Friday, July 15, 2016 8:01 AM
> > 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; H. Peter Anvin <hpa@zytor.com>; X86 ML
> > <x86@kernel.org>; ptyser@xes-inc.com; Lee Jones
> <lee.jones@linaro.org>;
> > Linus Walleij <linus.walleij@linaro.org>; linux-gpio@vger.kernel.org; LKML
> > <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 v6 1/3] x86/platform/p2sb: New Primary to Sideband
> > bridge support driver for Intel SOC's
> >
> > On Thu, Jul 14, 2016 at 4:11 AM, Tan Jui Nee <jui.nee.tan@intel.com>
> wrote:
> > > 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>
> > > ---
> > > Changes in V6:
> > >         - No change
> > >
> > > Changes in V5:
> > >         - No change
> > >
> > > Changes in V4:
> > >         - Move Kconfig option CONFIG_X86_INTEL_NON_ACPI from
> > >           [PATCH 2/3] x86/platform/p2sb: New Primary to Sideband bridge
> > support driver for Intel SOC's
> > >           to
> > >           [PATCH 3/3] mfd: lpc_ich: Add support for Intel Apollo Lake GPIO
> > pinctrl in non-ACPI system
> > >           since the config is used in latter patch.
> > >
> > > Changes in V3:
> > >         - No change
> > >
> > > Changes in V2:
> > >         - Add new config option CONFIG_X86_INTEL_NON_ACPI and "select
> > PINCTRL"
> > >           to fix kbuildbot error
> > >
> > >  arch/x86/Kconfig                 |  4 ++
> > >  arch/x86/include/asm/p2sb.h      | 27 +++++++++++
> > >  arch/x86/platform/intel/Makefile |  1 +
> > >  arch/x86/platform/intel/p2sb.c   | 99
> > ++++++++++++++++++++++++++++++++++++++++
> > >  4 files changed, 131 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
> > > d9a94da..d305d81 100644
> > > --- a/arch/x86/Kconfig
> > > +++ b/arch/x86/Kconfig
> > > @@ -604,6 +604,10 @@ config IOSF_MBI_DEBUG
> > >
> > >           If you don't require the option or are in doubt, say N.
> > >
> > > +config P2SB
> > > +       tristate
> >
> > OK, this is tristate, but then....
> >
> P2SB is tristate as currently it is only used by LPC_ICH that is tristate too.
> ...
> config LPC_ICH
> 	tristate "Intel ICH LPC"
> 	depends on X86 && PCI
> 	select MFD_CORE
> 	select P2SB
> ...
> > > +       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>
> >
> > ...and module.h is included, but yet...
> >
> > > +#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");
> >
> > ...the above is the only modular "use" that I can find.  So is the
> > tristate bogus?   Without a module_init and a module_exit I am
> > confused....
> >
> > I just finished an audit of arch/x86 for bogus uses of module.h so I'd like to
> > ensure we don't add more.
> >
> > Thanks,
> > Paul.
> > --
> >
> P2SB could be "bool" instead of tristate.
> My concern is if LPC_ICH built as module and not loaded, P2SB might
> consume memory when P2SB is not being used.
What I meant to say is, when LPC_ICH is built but not use by the current
hardware, the P2SB compiled code will occupy memory and cannot be
unloaded even if it is not in use. If memory usage is not the concern, I will
rework this patch to use Boolean option.
> What do you think? If that's ok for you, my next patch will be something like
> this:
> ...
> config P2SB
> 	bool
> 	depends on PCI
> ...
> In p2sb.c, module.h header file will be removed as well.
> Hi Andy, please provide your comments and/or concerns if any. Thanks.
> > > --
> > > 1.9.1
> > >

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

[Paul Gortmaker]

[RE: [PATCH v6 1/3] x86/platform/p2sb: New Primary to Sideband bridge support driver for Intel SOC's] On 18/07/2016 (Mon 03:35) Tan, Jui Nee wrote:

[...]

> > > +
> > > +       return 0;
> > > +}
> > > +EXPORT_SYMBOL(p2sb_bar);
> > > +
> > > +MODULE_LICENSE("GPL");
> > 
> > ...the above is the only modular "use" that I can find.  So is the
> > tristate bogus?   Without a module_init and a module_exit I am
> > confused....
> > 
> > I just finished an audit of arch/x86 for bogus uses of module.h so I'd like to
> > ensure we don't add more.
> > 
> > Thanks,
> > Paul.
> > --
> > 
> P2SB could be "bool" instead of tristate. 
> My concern is if LPC_ICH built as module and not loaded, P2SB might
> consume memory when P2SB is not being used.
> What do you think? If that's ok for you, my next patch will be something like
> this:
> ...
> config P2SB
> 	bool
> 	depends on PCI

After seeing that this module isn't using an init/exit to allocate and
free resources, I'd initially thought it was linked into part of a
bigger module file, in which case we could just delete the token
MODULE_LICENSE tag (since it adds zero value) and then delete the
module.h include too (but probably use export.h instead).

However, re-examining the Makefile, I see this does look to be used as a
single file module, (but more like a library vs. an active driver), so
the MODULE_LICENSE will be required to keep the kernel happy -- i.e. it
is fine as it was and my apologies for adding confusion.

Thanks,
Paul.
--

> ...
> In p2sb.c, module.h header file will be removed as well.
> Hi Andy, please provide your comments and/or concerns if any. Thanks.
> > > --
> > > 1.9.1
> > >

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

[Paul Gortmaker]

[RE: [PATCH v6 1/3] x86/platform/p2sb: New Primary to Sideband bridge support driver for Intel SOC's] On 18/07/2016 (Mon 03:35) Tan, Jui Nee wrote:

[...]

> > > +
> > > +       return 0;
> > > +}
> > > +EXPORT_SYMBOL(p2sb_bar);
> > > +
> > > +MODULE_LICENSE("GPL");
> > 
> > ...the above is the only modular "use" that I can find.  So is the
> > tristate bogus?   Without a module_init and a module_exit I am
> > confused....
> > 
> > I just finished an audit of arch/x86 for bogus uses of module.h so I'd like to
> > ensure we don't add more.
> > 
> > Thanks,
> > Paul.
> > --
> > 
> P2SB could be "bool" instead of tristate. 
> My concern is if LPC_ICH built as module and not loaded, P2SB might
> consume memory when P2SB is not being used.
> What do you think? If that's ok for you, my next patch will be something like
> this:
> ...
> config P2SB
> 	bool
> 	depends on PCI

After seeing that this module isn't using an init/exit to allocate and
free resources, I'd initially thought it was linked into part of a
bigger module file, in which case we could just delete the token
MODULE_LICENSE tag (since it adds zero value) and then delete the
module.h include too (but probably use export.h instead).

However, re-examining the Makefile, I see this does look to be used as a
single file module, (but more like a library vs. an active driver), so
the MODULE_LICENSE will be required to keep the kernel happy -- i.e. it
is fine as it was and my apologies for adding confusion.

Thanks,
Paul.
--

> ...
> In p2sb.c, module.h header file will be removed as well.
> Hi Andy, please provide your comments and/or concerns if any. Thanks.
> > > --
> > > 1.9.1
> > >

Re: [PATCH v6 2/3] mfd: lpc_ich: Rename lpc-ich driver

[Lee Jones]

On Thu, 14 Jul 2016, Tan Jui Nee wrote:

> This patch follows the example of mfd/wm831x to rename the driver
> from "lpc_ich" to "lpc_ich-core".
> 
> Signed-off-by: Tan Jui Nee <jui.nee.tan@intel.com>
> ---
> Changes in V6:
> 	- none, just a subject line and commit message change.
> 
>  drivers/mfd/Makefile                      | 1 +
>  drivers/mfd/{lpc_ich.c => lpc_ich-core.c} | 0
>  2 files changed, 1 insertion(+)
>  rename drivers/mfd/{lpc_ich.c => lpc_ich-core.c} (100%)

Looks a bit odd.

I suggest 'lpc_ich_core.c' instead.

> diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
> index 42a66e1..1dfe5fb 100644
> --- a/drivers/mfd/Makefile
> +++ b/drivers/mfd/Makefile
> @@ -155,6 +155,7 @@ obj-$(CONFIG_PMIC_ADP5520)	+= adp5520.o
>  obj-$(CONFIG_MFD_KEMPLD)	+= kempld-core.o
>  obj-$(CONFIG_MFD_INTEL_QUARK_I2C_GPIO)	+= intel_quark_i2c_gpio.o
>  obj-$(CONFIG_LPC_SCH)		+= lpc_sch.o
> +lpc_ich-objs		:= lpc_ich-core.o
>  obj-$(CONFIG_LPC_ICH)		+= lpc_ich.o
>  obj-$(CONFIG_MFD_RDC321X)	+= rdc321x-southbridge.o
>  obj-$(CONFIG_MFD_JANZ_CMODIO)	+= janz-cmodio.o
> diff --git a/drivers/mfd/lpc_ich.c b/drivers/mfd/lpc_ich-core.c
> similarity index 100%
> rename from drivers/mfd/lpc_ich.c
> rename to drivers/mfd/lpc_ich-core.c

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

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

[Lee Jones]

On Thu, 14 Jul 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>
> ---
> Changes in V6:
> 	- Rename CONFIG_X86_INTEL_APL to CONFIG_X86_INTEL_IVI so that it
> 	  relates to the actual product, as suggested by Mika.
> 	- Rework Makefile according Andy's comments.
> 	- Rename lpc_ich_misc() to lpc_ich_add_gpio() so that the name should not
> 	  be so generic, as suggested by Andy.
> 	- Call lpc_ich_add_gpio() via priv->chipset.
> 	- lpc_ich_add_gpio() function will be moved from 
> 	  .../include/linux/mfd/lpc_ich.h to
> 	  .../drivers/mfd/lpc_ich-apl.h
> 	  as this is a part of internal driver interface as suggested by Andy.
> 	- Move enum lpc_chipsets from 
> 	  .../drivers/mfd/lpc_ich-core.c to
> 	  .../include/linux/mfd/lpc_ich.h
> 	  as lpc_chipsets is also accessed by lpc_ich_add_gpio().
> 	- Check if kasprintf return value for all 4 gpio controllers before
> 	  proceed to add platform device by using mfd_add_devices().
> 
> Changes in V5:
> 	- Split lpc-ich driver into two parts (lpc_ich-core and lpc_ich-apl).
> 	  The file lpc_ich-apl.c introduces gpio platform driver in MFD.
> 	- Rename Kconfig option CONFIG_X86_INTEL_NON_ACPI to CONFIG_X86_INTEL_APL
> 	  so that it reflects actual product as suggested by Mika.
> 
> Changes in V4:
> 	- Move Kconfig option CONFIG_X86_INTEL_NON_ACPI from
> 	  [PATCH 2/3] x86/platform/p2sb: New Primary to Sideband bridge support driver for Intel SOC's
> 	  to
> 	  [PATCH 3/3] mfd: lpc_ich: Add support for Intel Apollo Lake GPIO pinctrl in non-ACPI system
> 	  since the config is used in latter patch.
> 	- Select CONFIG_P2SB when CONFIG_LPC_ICH is enabled.
> 	- Remove #ifdef CONFIG_X86_INTEL_NON_ACPI and use
> 	  #if defined(CONFIG_X86_INTEL_NON_ACPI) when lpc_ich_misc is called
> 	  as suggested by Lee Jones.
> 	- Use single dimensional array instead of 2D array for apl_gpio_io_res
> 	  structure and use DEFINE_RES_IRQ for its IRQ resource.
> 
> Changes in V3:
> 	- Simplify register addresses calculation and use DEFINE_RES_MEM_NAMED
> 	  defines for apl_gpio_io_res structure
> 	- Define magic number for P2SB PCI ID
> 	- Replace switch-case with if-else since currently we have only one
> 	  use case
> 	- Only call mfd_add_devices() once for all gpio communities
> 
> Changes in V2:
> 	- Add new config option CONFIG_X86_INTEL_NON_ACPI and "select PINCTRL"
> 	  to fix kbuildbot error
> 
>  arch/x86/Kconfig            |   9 +++
>  drivers/mfd/Kconfig         |   3 +-
>  drivers/mfd/Makefile        |   5 +-
>  drivers/mfd/lpc_ich-apl.c   | 130 ++++++++++++++++++++++++++++++++++++++++++++
>  drivers/mfd/lpc_ich-apl.h   |  28 ++++++++++
>  drivers/mfd/lpc_ich-core.c  |  81 ++++-----------------------
>  include/linux/mfd/lpc_ich.h |  73 +++++++++++++++++++++++++
>  7 files changed, 256 insertions(+), 73 deletions(-)
>  create mode 100644 drivers/mfd/lpc_ich-apl.c
>  create mode 100644 drivers/mfd/lpc_ich-apl.h
> 
> diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
> index d305d81..c0b427b 100644
> --- a/arch/x86/Kconfig
> +++ b/arch/x86/Kconfig
> @@ -513,6 +513,15 @@ config X86_INTEL_CE
>  	  This option compiles in support for the CE4100 SOC for settop
>  	  boxes and media devices.
>  
> +config X86_INTEL_IVI
> +	bool "Intel in-vehicle infotainment (IVI) systems used in cars"
> +	select PINCTRL
> +	---help---
> +	  Select this option to enable MMIO BAR access over the P2SB for
> +	  non-ACPI Intel Apollo Lake SoC platforms. This driver uses the P2SB
> +	  hide/unhide mechanism cooperatively to pass the PCI BAR address to
> +	  the platform driver, currently GPIO.
> +

This should be a separate patch.

>  config X86_INTEL_MID
>  	bool "Intel MID platform support"
>  	depends on X86_EXTENDED_PLATFORM
> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> index 1bcf601..dc4e543 100644
> --- a/drivers/mfd/Kconfig
> +++ b/drivers/mfd/Kconfig
> @@ -369,8 +369,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
>  	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/Makefile b/drivers/mfd/Makefile
> index 1dfe5fb..0aa3e1f 100644
> --- a/drivers/mfd/Makefile
> +++ b/drivers/mfd/Makefile
> @@ -155,8 +155,11 @@ obj-$(CONFIG_PMIC_ADP5520)	+= adp5520.o
>  obj-$(CONFIG_MFD_KEMPLD)	+= kempld-core.o
>  obj-$(CONFIG_MFD_INTEL_QUARK_I2C_GPIO)	+= intel_quark_i2c_gpio.o
>  obj-$(CONFIG_LPC_SCH)		+= lpc_sch.o
> -lpc_ich-objs		:= lpc_ich-core.o
>  obj-$(CONFIG_LPC_ICH)		+= lpc_ich.o
> +lpc_ich-objs		:= lpc_ich-core.o
> +ifeq ($(CONFIG_X86_INTEL_IVI),y)
> +lpc_ich-objs += lpc_ich-apl.o
> +endif
>  obj-$(CONFIG_MFD_RDC321X)	+= rdc321x-southbridge.o
>  obj-$(CONFIG_MFD_JANZ_CMODIO)	+= janz-cmodio.o
>  obj-$(CONFIG_MFD_JZ4740_ADC)	+= jz4740-adc.o
> diff --git a/drivers/mfd/lpc_ich-apl.c b/drivers/mfd/lpc_ich-apl.c
> new file mode 100644
> index 0000000..e4b9688
> --- /dev/null
> +++ b/drivers/mfd/lpc_ich-apl.c
> @@ -0,0 +1,130 @@
> +/*
> + * Purpose: Create a platform device to bind with Intel Apollo Lake

Never seen this before.  Looks like more of a commit log entry than
something that should live in a source file header.

> + * Pinctrl GPIO platform driver

No need to mention "platform driver" here. Most drivers are.

> + * Copyright (C) 2016 Intel Corporation

Author?

> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + */
> +
> +#include <linux/pci.h>
> +#include <linux/mfd/core.h>
> +#include <linux/mfd/lpc_ich.h>
> +#include <linux/pinctrl/pinctrl.h>
> +#include <asm/p2sb.h>

Alphabetical.

> +#include "lpc_ich-apl.h"

Don't' mix '_'s with '-'s.

> +/* Offset data for Apollo Lake GPIO communities */
> +#define APL_GPIO_SOUTHWEST_OFFSET	0xc00000
> +#define APL_GPIO_NORTHWEST_OFFSET	0xc40000
> +#define APL_GPIO_NORTH_OFFSET		0xc50000
> +#define APL_GPIO_WEST_OFFSET		0xc70000
> +
> +#define APL_GPIO_SOUTHWEST_NPIN		43
> +#define APL_GPIO_NORTHWEST_NPIN		77
> +#define APL_GPIO_NORTH_NPIN		78
> +#define APL_GPIO_WEST_NPIN		47
> +
> +#define APL_GPIO_IRQ 14
> +
> +#define PCI_IDSEL_P2SB	0x0d
> +
> +static struct resource apl_gpio_io_res[] = {
> +	DEFINE_RES_MEM_NAMED(APL_GPIO_NORTH_OFFSET,
> +		APL_GPIO_NORTH_NPIN * SZ_8, "apl_pinctrl_n"),
> +	DEFINE_RES_MEM_NAMED(APL_GPIO_NORTHWEST_OFFSET,
> +		APL_GPIO_NORTHWEST_NPIN * SZ_8, "apl_pinctrl_nw"),
> +	DEFINE_RES_MEM_NAMED(APL_GPIO_WEST_OFFSET,
> +		APL_GPIO_WEST_NPIN * SZ_8, "apl_pinctrl_w"),
> +	DEFINE_RES_MEM_NAMED(APL_GPIO_SOUTHWEST_OFFSET,
> +		APL_GPIO_SOUTHWEST_NPIN * SZ_8, "apl_pinctrl_sw"),
> +	DEFINE_RES_IRQ(APL_GPIO_IRQ),
> +};
> +
> +static struct pinctrl_pin_desc apl_pinctrl_pdata;

This seems pretty pointless.  What's it for?

> +static struct mfd_cell apl_gpio_devices[] = {
> +	{
> +		.name = "apl-pinctrl",
> +		.id = 0,
> +		.num_resources = ARRAY_SIZE(apl_gpio_io_res),
> +		.resources = apl_gpio_io_res,
> +		.pdata_size = sizeof(apl_pinctrl_pdata),
> +		.platform_data = &apl_pinctrl_pdata,
> +		.ignore_resource_conflicts = true,
> +	},
> +	{
> +		.name = "apl-pinctrl",
> +		.id = 1,
> +		.num_resources = ARRAY_SIZE(apl_gpio_io_res),
> +		.resources = apl_gpio_io_res,
> +		.pdata_size = sizeof(apl_pinctrl_pdata),
> +		.platform_data = &apl_pinctrl_pdata,
> +		.ignore_resource_conflicts = true,
> +	},
> +	{
> +		.name = "apl-pinctrl",
> +		.id = 2,
> +		.num_resources = ARRAY_SIZE(apl_gpio_io_res),
> +		.resources = apl_gpio_io_res,
> +		.pdata_size = sizeof(apl_pinctrl_pdata),
> +		.platform_data = &apl_pinctrl_pdata,
> +		.ignore_resource_conflicts = true,
> +	},
> +	{
> +		.name = "apl-pinctrl",
> +		.id = 3,
> +		.num_resources = ARRAY_SIZE(apl_gpio_io_res),
> +		.resources = apl_gpio_io_res,
> +		.pdata_size = sizeof(apl_pinctrl_pdata),
> +		.platform_data = &apl_pinctrl_pdata,
> +		.ignore_resource_conflicts = true,
> +	},
> +};
> +
> +int lpc_ich_add_gpio(struct pci_dev *dev, enum lpc_chipsets chipset)
> +{
> +	unsigned int apl_p2sb = PCI_DEVFN(PCI_IDSEL_P2SB, 0);

You only use this variable one.  Just switch it out for the macro.

> +	unsigned int i;
> +	int ret;
> +
> +	if (chipset != LPC_APL)
> +		return -ENODEV;
> +	/*
> +	 * Apollo lake, has not 1, but 4 gpio controllers,
> +	 * handle it a bit differently.
> +	 */
> +
> +	for (i = 0; i < ARRAY_SIZE(apl_gpio_io_res)-1; i++) {

This is fragile.  Just define a GPIO_CONTROLLER_COUNT or similar.

> +		struct resource *res = &apl_gpio_io_res[i];
> +
> +		apl_gpio_devices[i].resources = res;

What's happening here?  Are you manually pulling resources out of the
resource structure and linking them to the device cells?

Why?

> +		/* Fill MEM resource */
> +		ret = p2sb_bar(dev, apl_p2sb, res++);

Why res++?

> +		if (ret)
> +			goto warn_continue;
> +
> +		apl_pinctrl_pdata.name = kasprintf(GFP_KERNEL, "%u",
> +			i + 1);

Are you sure this does what you think it does?


I think you will keep changing *.name, and the only one you will
see/use is the last one.

Have you tested this?

> +		if (apl_pinctrl_pdata.name == NULL) {
> +			ret = -ENOMEM;
> +			goto warn_continue;

No, this is a failure.  You need to 'error' out and return ret.

> +		}
> +	}
> +
> +	ret = mfd_add_devices(&dev->dev, 0,
> +		apl_gpio_devices, ARRAY_SIZE(apl_gpio_devices),
> +			NULL, 0, NULL);
> +
> +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);

You've just passed the child device a NULL pointer.

> +	return 0;

Why are you returning 0 even during failure?

> +}
> diff --git a/drivers/mfd/lpc_ich-apl.h b/drivers/mfd/lpc_ich-apl.h
> new file mode 100644
> index 0000000..db8325d
> --- /dev/null
> +++ b/drivers/mfd/lpc_ich-apl.h
> @@ -0,0 +1,28 @@
> +/*
> + * lpc_ich-apl.h - Intel in-vehicle infotainment (IVI) systems used in cars
> + *                 support

The "In", "Vehicle" and "Infotainment" should be capitalised.

> + * Copyright (C) 2016, Intel Corporation
> + *
> + * Authors: Tan, Jui Nee <jui.nee.tan@intel.com>

"Author"

> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + */
> +
> +#ifndef __LPC_ICH_APL_H__
> +#define __LPC_ICH_APL_H__
> +
> +#include <linux/pci.h>
> +
> +#if IS_ENABLED(CONFIG_X86_INTEL_IVI)
> +int lpc_ich_add_gpio(struct pci_dev *dev, enum lpc_chipsets chipset);
> +#else /* CONFIG_X86_INTEL_IVI is not set */
> +static inline int lpc_ich_add_gpio(struct pci_dev *dev, enum lpc_chipsets chipset)
> +{
> +	return -ENODEV;
> +}
> +#endif
> +
> +#endif
> diff --git a/drivers/mfd/lpc_ich-core.c b/drivers/mfd/lpc_ich-core.c
> index bd3aa45..e09d1e2 100644
> --- a/drivers/mfd/lpc_ich-core.c
> +++ b/drivers/mfd/lpc_ich-core.c
> @@ -69,6 +69,8 @@
>  #include <linux/mfd/lpc_ich.h>
>  #include <linux/platform_data/itco_wdt.h>
>  
> +#include "lpc_ich-apl.h"
> +
>  #define ACPIBASE		0x40
>  #define ACPIBASE_GPE_OFF	0x28
>  #define ACPIBASE_GPE_END	0x2f
> @@ -147,77 +149,6 @@ static struct mfd_cell lpc_ich_gpio_cell = {
>  	.ignore_resource_conflicts = true,
>  };
>  
> -/* chipset related info */
> -enum lpc_chipsets {
> -	LPC_ICH = 0,	/* ICH */
> -	LPC_ICH0,	/* ICH0 */
> -	LPC_ICH2,	/* ICH2 */
> -	LPC_ICH2M,	/* ICH2-M */
> -	LPC_ICH3,	/* ICH3-S */
> -	LPC_ICH3M,	/* ICH3-M */
> -	LPC_ICH4,	/* ICH4 */
> -	LPC_ICH4M,	/* ICH4-M */
> -	LPC_CICH,	/* C-ICH */
> -	LPC_ICH5,	/* ICH5 & ICH5R */
> -	LPC_6300ESB,	/* 6300ESB */
> -	LPC_ICH6,	/* ICH6 & ICH6R */
> -	LPC_ICH6M,	/* ICH6-M */
> -	LPC_ICH6W,	/* ICH6W & ICH6RW */
> -	LPC_631XESB,	/* 631xESB/632xESB */
> -	LPC_ICH7,	/* ICH7 & ICH7R */
> -	LPC_ICH7DH,	/* ICH7DH */
> -	LPC_ICH7M,	/* ICH7-M & ICH7-U */
> -	LPC_ICH7MDH,	/* ICH7-M DH */
> -	LPC_NM10,	/* NM10 */
> -	LPC_ICH8,	/* ICH8 & ICH8R */
> -	LPC_ICH8DH,	/* ICH8DH */
> -	LPC_ICH8DO,	/* ICH8DO */
> -	LPC_ICH8M,	/* ICH8M */
> -	LPC_ICH8ME,	/* ICH8M-E */
> -	LPC_ICH9,	/* ICH9 */
> -	LPC_ICH9R,	/* ICH9R */
> -	LPC_ICH9DH,	/* ICH9DH */
> -	LPC_ICH9DO,	/* ICH9DO */
> -	LPC_ICH9M,	/* ICH9M */
> -	LPC_ICH9ME,	/* ICH9M-E */
> -	LPC_ICH10,	/* ICH10 */
> -	LPC_ICH10R,	/* ICH10R */
> -	LPC_ICH10D,	/* ICH10D */
> -	LPC_ICH10DO,	/* ICH10DO */
> -	LPC_PCH,	/* PCH Desktop Full Featured */
> -	LPC_PCHM,	/* PCH Mobile Full Featured */
> -	LPC_P55,	/* P55 */
> -	LPC_PM55,	/* PM55 */
> -	LPC_H55,	/* H55 */
> -	LPC_QM57,	/* QM57 */
> -	LPC_H57,	/* H57 */
> -	LPC_HM55,	/* HM55 */
> -	LPC_Q57,	/* Q57 */
> -	LPC_HM57,	/* HM57 */
> -	LPC_PCHMSFF,	/* PCH Mobile SFF Full Featured */
> -	LPC_QS57,	/* QS57 */
> -	LPC_3400,	/* 3400 */
> -	LPC_3420,	/* 3420 */
> -	LPC_3450,	/* 3450 */
> -	LPC_EP80579,	/* EP80579 */
> -	LPC_CPT,	/* Cougar Point */
> -	LPC_CPTD,	/* Cougar Point Desktop */
> -	LPC_CPTM,	/* Cougar Point Mobile */
> -	LPC_PBG,	/* Patsburg */
> -	LPC_DH89XXCC,	/* DH89xxCC */
> -	LPC_PPT,	/* Panther Point */
> -	LPC_LPT,	/* Lynx Point */
> -	LPC_LPT_LP,	/* Lynx Point-LP */
> -	LPC_WBG,	/* Wellsburg */
> -	LPC_AVN,	/* Avoton SoC */
> -	LPC_BAYTRAIL,   /* Bay Trail SoC */
> -	LPC_COLETO,	/* Coleto Creek */
> -	LPC_WPT_LP,	/* Wildcat Point-LP */
> -	LPC_BRASWELL,	/* Braswell SoC */
> -	LPC_LEWISBURG,	/* Lewisburg */
> -	LPC_9S,		/* 9 Series */
> -};

What does this move have to do with this patch?

I think it should be separate and be paired with a reason for the
change.

>  static struct lpc_ich_info lpc_chipset_info[] = {
>  	[LPC_ICH] = {
>  		.name = "ICH",
> @@ -531,6 +462,10 @@ static struct lpc_ich_info lpc_chipset_info[] = {
>  		.name = "9 Series",
>  		.iTCO_version = 2,
>  	},
> +	[LPC_APL]  = {
> +		.name = "Apollo Lake SoC",
> +		.iTCO_version = 5,
> +	},

Enabling a new platform should be a separate patch.

>  };
>  
>  /*
> @@ -679,6 +614,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},
> @@ -1093,6 +1029,9 @@ static int lpc_ich_probe(struct pci_dev *dev,
>  			cell_added = true;
>  	}
>  
> +	if (!lpc_ich_add_gpio(dev, priv->chipset))
> +		cell_added = true;
> +
>  	/*
>  	 * We only care if at least one or none of the cells registered
>  	 * successfully.
> diff --git a/include/linux/mfd/lpc_ich.h b/include/linux/mfd/lpc_ich.h
> index 2b300b4..03c2ca3 100644
> --- a/include/linux/mfd/lpc_ich.h
> +++ b/include/linux/mfd/lpc_ich.h
> @@ -34,6 +34,7 @@ enum {
>  	ICH_V10CORP_GPIO,
>  	ICH_V10CONS_GPIO,
>  	AVOTON_GPIO,
> +	APL_GPIO,
>  };
>  
>  struct lpc_ich_info {
> @@ -43,4 +44,76 @@ struct lpc_ich_info {
>  	u8 use_gpio;
>  };
>  
> +/* chipset related info */
> +enum lpc_chipsets {
> +	LPC_ICH = 0,	/* ICH */
> +	LPC_ICH0,	/* ICH0 */
> +	LPC_ICH2,	/* ICH2 */
> +	LPC_ICH2M,	/* ICH2-M */
> +	LPC_ICH3,	/* ICH3-S */
> +	LPC_ICH3M,	/* ICH3-M */
> +	LPC_ICH4,	/* ICH4 */
> +	LPC_ICH4M,	/* ICH4-M */
> +	LPC_CICH,	/* C-ICH */
> +	LPC_ICH5,	/* ICH5 & ICH5R */
> +	LPC_6300ESB,	/* 6300ESB */
> +	LPC_ICH6,	/* ICH6 & ICH6R */
> +	LPC_ICH6M,	/* ICH6-M */
> +	LPC_ICH6W,	/* ICH6W & ICH6RW */
> +	LPC_631XESB,	/* 631xESB/632xESB */
> +	LPC_ICH7,	/* ICH7 & ICH7R */
> +	LPC_ICH7DH,	/* ICH7DH */
> +	LPC_ICH7M,	/* ICH7-M & ICH7-U */
> +	LPC_ICH7MDH,	/* ICH7-M DH */
> +	LPC_NM10,	/* NM10 */
> +	LPC_ICH8,	/* ICH8 & ICH8R */
> +	LPC_ICH8DH,	/* ICH8DH */
> +	LPC_ICH8DO,	/* ICH8DO */
> +	LPC_ICH8M,	/* ICH8M */
> +	LPC_ICH8ME,	/* ICH8M-E */
> +	LPC_ICH9,	/* ICH9 */
> +	LPC_ICH9R,	/* ICH9R */
> +	LPC_ICH9DH,	/* ICH9DH */
> +	LPC_ICH9DO,	/* ICH9DO */
> +	LPC_ICH9M,	/* ICH9M */
> +	LPC_ICH9ME,	/* ICH9M-E */
> +	LPC_ICH10,	/* ICH10 */
> +	LPC_ICH10R,	/* ICH10R */
> +	LPC_ICH10D,	/* ICH10D */
> +	LPC_ICH10DO,	/* ICH10DO */
> +	LPC_PCH,	/* PCH Desktop Full Featured */
> +	LPC_PCHM,	/* PCH Mobile Full Featured */
> +	LPC_P55,	/* P55 */
> +	LPC_PM55,	/* PM55 */
> +	LPC_H55,	/* H55 */
> +	LPC_QM57,	/* QM57 */
> +	LPC_H57,	/* H57 */
> +	LPC_HM55,	/* HM55 */
> +	LPC_Q57,	/* Q57 */
> +	LPC_HM57,	/* HM57 */
> +	LPC_PCHMSFF,	/* PCH Mobile SFF Full Featured */
> +	LPC_QS57,	/* QS57 */
> +	LPC_3400,	/* 3400 */
> +	LPC_3420,	/* 3420 */
> +	LPC_3450,	/* 3450 */
> +	LPC_EP80579,	/* EP80579 */
> +	LPC_CPT,	/* Cougar Point */
> +	LPC_CPTD,	/* Cougar Point Desktop */
> +	LPC_CPTM,	/* Cougar Point Mobile */
> +	LPC_PBG,	/* Patsburg */
> +	LPC_DH89XXCC,	/* DH89xxCC */
> +	LPC_PPT,	/* Panther Point */
> +	LPC_LPT,	/* Lynx Point */
> +	LPC_LPT_LP,	/* Lynx Point-LP */
> +	LPC_WBG,	/* Wellsburg */
> +	LPC_AVN,	/* Avoton SoC */
> +	LPC_BAYTRAIL,   /* Bay Trail SoC */
> +	LPC_COLETO,	/* Coleto Creek */
> +	LPC_WPT_LP,	/* Wildcat Point-LP */
> +	LPC_BRASWELL,	/* Braswell SoC */
> +	LPC_LEWISBURG,	/* Lewisburg */
> +	LPC_9S,		/* 9 Series */
> +	LPC_APL,	/* Apollo Lake SoC */
> +};
> +
>  #endif

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

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

[Andy Shevchenko]

On Mon, 2016-07-18 at 03:35 +0000, Tan, Jui Nee wrote:
> 
> > 
> > +
> > > +MODULE_LICENSE("GPL");
> > 
> > ...the above is the only modular "use" that I can find.  So is the
> > tristate bogus?   Without a module_init and a module_exit I am
> > confused....
> > 
> > I just finished an audit of arch/x86 for bogus uses of module.h so
> > I'd like to
> > ensure we don't add more.

It's a library that can be used as module. My concern is memory
footprint of non-used libraries and modules. I prefer to let module be
tristate until it's proved that it can't be.

> P2SB could be "bool" instead of tristate. 
> My concern is if LPC_ICH built as module and not loaded, P2SB might
> consume memory when P2SB is not being used.
> What do you think? If that's ok for you, my next patch will be
> something like
> this:
> ...
> config P2SB
> 	bool
> 	depends on PCI
> ...
> In p2sb.c, module.h header file will be removed as well.
> Hi Andy, please provide your comments and/or concerns if any. Thanks.


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

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

[Andy Shevchenko]

On Mon, 2016-07-18 at 03:35 +0000, Tan, Jui Nee wrote:
> 
> > 
> > +
> > > +MODULE_LICENSE("GPL");
> > 
> > ...the above is the only modular "use" that I can find.  So is the
> > tristate bogus?   Without a module_init and a module_exit I am
> > confused....
> > 
> > I just finished an audit of arch/x86 for bogus uses of module.h so
> > I'd like to
> > ensure we don't add more.

It's a library that can be used as module. My concern is memory
footprint of non-used libraries and modules. I prefer to let module be
tristate until it's proved that it can't be.

> P2SB could be "bool" instead of tristate. 
> My concern is if LPC_ICH built as module and not loaded, P2SB might
> consume memory when P2SB is not being used.
> What do you think? If that's ok for you, my next patch will be
> something like
> this:
> ...
> config P2SB
> 	bool
> 	depends on PCI
> ...
> In p2sb.c, module.h header file will be removed as well.
> Hi Andy, please provide your comments and/or concerns if any. Thanks.


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

RE: [PATCH v6 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: Tuesday, August 9, 2016 3:16 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;
> linus.walleij@linaro.org; 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 v6 3/3] mfd: lpc_ich: Add support for Intel Apollo Lake
> GPIO pinctrl in non-ACPI system
> 
> On Thu, 14 Jul 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>
> > ---
> > Changes in V6:
> > 	- Rename CONFIG_X86_INTEL_APL to CONFIG_X86_INTEL_IVI so
> that it
> > 	  relates to the actual product, as suggested by Mika.
> > 	- Rework Makefile according Andy's comments.
> > 	- Rename lpc_ich_misc() to lpc_ich_add_gpio() so that the name
> should not
> > 	  be so generic, as suggested by Andy.
> > 	- Call lpc_ich_add_gpio() via priv->chipset.
> > 	- lpc_ich_add_gpio() function will be moved from
> > 	  .../include/linux/mfd/lpc_ich.h to
> > 	  .../drivers/mfd/lpc_ich-apl.h
> > 	  as this is a part of internal driver interface as suggested by Andy.
> > 	- Move enum lpc_chipsets from
> > 	  .../drivers/mfd/lpc_ich-core.c to
> > 	  .../include/linux/mfd/lpc_ich.h
> > 	  as lpc_chipsets is also accessed by lpc_ich_add_gpio().
> > 	- Check if kasprintf return value for all 4 gpio controllers before
> > 	  proceed to add platform device by using mfd_add_devices().
> >
> > Changes in V5:
> > 	- Split lpc-ich driver into two parts (lpc_ich-core and lpc_ich-apl).
> > 	  The file lpc_ich-apl.c introduces gpio platform driver in MFD.
> > 	- Rename Kconfig option CONFIG_X86_INTEL_NON_ACPI to
> CONFIG_X86_INTEL_APL
> > 	  so that it reflects actual product as suggested by Mika.
> >
> > Changes in V4:
> > 	- Move Kconfig option CONFIG_X86_INTEL_NON_ACPI from
> > 	  [PATCH 2/3] x86/platform/p2sb: New Primary to Sideband bridge
> support driver for Intel SOC's
> > 	  to
> > 	  [PATCH 3/3] mfd: lpc_ich: Add support for Intel Apollo Lake GPIO
> pinctrl in non-ACPI system
> > 	  since the config is used in latter patch.
> > 	- Select CONFIG_P2SB when CONFIG_LPC_ICH is enabled.
> > 	- Remove #ifdef CONFIG_X86_INTEL_NON_ACPI and use
> > 	  #if defined(CONFIG_X86_INTEL_NON_ACPI) when lpc_ich_misc is
> called
> > 	  as suggested by Lee Jones.
> > 	- Use single dimensional array instead of 2D array for apl_gpio_io_res
> > 	  structure and use DEFINE_RES_IRQ for its IRQ resource.
> >
> > Changes in V3:
> > 	- Simplify register addresses calculation and use
> DEFINE_RES_MEM_NAMED
> > 	  defines for apl_gpio_io_res structure
> > 	- Define magic number for P2SB PCI ID
> > 	- Replace switch-case with if-else since currently we have only one
> > 	  use case
> > 	- Only call mfd_add_devices() once for all gpio communities
> >
> > Changes in V2:
> > 	- Add new config option CONFIG_X86_INTEL_NON_ACPI and "select
> PINCTRL"
> > 	  to fix kbuildbot error
> >
> >  arch/x86/Kconfig            |   9 +++
> >  drivers/mfd/Kconfig         |   3 +-
> >  drivers/mfd/Makefile        |   5 +-
> >  drivers/mfd/lpc_ich-apl.c   | 130
> ++++++++++++++++++++++++++++++++++++++++++++
> >  drivers/mfd/lpc_ich-apl.h   |  28 ++++++++++
> >  drivers/mfd/lpc_ich-core.c  |  81 ++++-----------------------
> > include/linux/mfd/lpc_ich.h |  73 +++++++++++++++++++++++++
> >  7 files changed, 256 insertions(+), 73 deletions(-)  create mode
> > 100644 drivers/mfd/lpc_ich-apl.c  create mode 100644
> > drivers/mfd/lpc_ich-apl.h
> >
> > diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig index
> > d305d81..c0b427b 100644
> > --- a/arch/x86/Kconfig
> > +++ b/arch/x86/Kconfig
> > @@ -513,6 +513,15 @@ config X86_INTEL_CE
> >  	  This option compiles in support for the CE4100 SOC for settop
> >  	  boxes and media devices.
> >
> > +config X86_INTEL_IVI
> > +	bool "Intel in-vehicle infotainment (IVI) systems used in cars"
> > +	select PINCTRL
> > +	---help---
> > +	  Select this option to enable MMIO BAR access over the P2SB for
> > +	  non-ACPI Intel Apollo Lake SoC platforms. This driver uses the P2SB
> > +	  hide/unhide mechanism cooperatively to pass the PCI BAR address
> to
> > +	  the platform driver, currently GPIO.
> > +
> 
> This should be a separate patch.
> 
Thanks for your comment. The changes will be applied in next patch-set.
> >  config X86_INTEL_MID
> >  	bool "Intel MID platform support"
> >  	depends on X86_EXTENDED_PLATFORM
> > diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig index
> > 1bcf601..dc4e543 100644
> > --- a/drivers/mfd/Kconfig
> > +++ b/drivers/mfd/Kconfig
> > @@ -369,8 +369,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
> >  	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/Makefile b/drivers/mfd/Makefile index
> > 1dfe5fb..0aa3e1f 100644
> > --- a/drivers/mfd/Makefile
> > +++ b/drivers/mfd/Makefile
> > @@ -155,8 +155,11 @@ obj-$(CONFIG_PMIC_ADP5520)	+= adp5520.o
> >  obj-$(CONFIG_MFD_KEMPLD)	+= kempld-core.o
> >  obj-$(CONFIG_MFD_INTEL_QUARK_I2C_GPIO)	+=
> intel_quark_i2c_gpio.o
> >  obj-$(CONFIG_LPC_SCH)		+= lpc_sch.o
> > -lpc_ich-objs		:= lpc_ich-core.o
> >  obj-$(CONFIG_LPC_ICH)		+= lpc_ich.o
> > +lpc_ich-objs		:= lpc_ich-core.o
> > +ifeq ($(CONFIG_X86_INTEL_IVI),y)
> > +lpc_ich-objs += lpc_ich-apl.o
> > +endif
> >  obj-$(CONFIG_MFD_RDC321X)	+= rdc321x-southbridge.o
> >  obj-$(CONFIG_MFD_JANZ_CMODIO)	+= janz-cmodio.o
> >  obj-$(CONFIG_MFD_JZ4740_ADC)	+= jz4740-adc.o
> > diff --git a/drivers/mfd/lpc_ich-apl.c b/drivers/mfd/lpc_ich-apl.c new
> > file mode 100644 index 0000000..e4b9688
> > --- /dev/null
> > +++ b/drivers/mfd/lpc_ich-apl.c
> > @@ -0,0 +1,130 @@
> > +/*
> > + * Purpose: Create a platform device to bind with Intel Apollo Lake
> 
> Never seen this before.  Looks like more of a commit log entry than
> something that should live in a source file header.
> 
> > + * Pinctrl GPIO platform driver
> 
> No need to mention "platform driver" here. Most drivers are.
> 
Okay, the changes will be applied in next patch-set.
> > + * Copyright (C) 2016 Intel Corporation
> 
> Author?
> 
I will add the Author information in next patch-set.
> > + * This program is free software; you can redistribute it and/or
> > + modify
> > + * it under the terms of the GNU General Public License version 2 as
> > + * published by the Free Software Foundation.
> > + */
> > +
> > +#include <linux/pci.h>
> > +#include <linux/mfd/core.h>
> > +#include <linux/mfd/lpc_ich.h>
> > +#include <linux/pinctrl/pinctrl.h>
> > +#include <asm/p2sb.h>
> 
> Alphabetical.
> 
Thanks for pointing that out.
> > +#include "lpc_ich-apl.h"
> 
> Don't' mix '_'s with '-'s.
> 
The header file will be renamed as lpc_ich_apl.h.
> > +/* Offset data for Apollo Lake GPIO communities */
> > +#define APL_GPIO_SOUTHWEST_OFFSET	0xc00000
> > +#define APL_GPIO_NORTHWEST_OFFSET	0xc40000
> > +#define APL_GPIO_NORTH_OFFSET		0xc50000
> > +#define APL_GPIO_WEST_OFFSET		0xc70000
> > +
> > +#define APL_GPIO_SOUTHWEST_NPIN		43
> > +#define APL_GPIO_NORTHWEST_NPIN		77
> > +#define APL_GPIO_NORTH_NPIN		78
> > +#define APL_GPIO_WEST_NPIN		47
> > +
> > +#define APL_GPIO_IRQ 14
> > +
> > +#define PCI_IDSEL_P2SB	0x0d
> > +
> > +static struct resource apl_gpio_io_res[] = {
> > +	DEFINE_RES_MEM_NAMED(APL_GPIO_NORTH_OFFSET,
> > +		APL_GPIO_NORTH_NPIN * SZ_8, "apl_pinctrl_n"),
> > +	DEFINE_RES_MEM_NAMED(APL_GPIO_NORTHWEST_OFFSET,
> > +		APL_GPIO_NORTHWEST_NPIN * SZ_8, "apl_pinctrl_nw"),
> > +	DEFINE_RES_MEM_NAMED(APL_GPIO_WEST_OFFSET,
> > +		APL_GPIO_WEST_NPIN * SZ_8, "apl_pinctrl_w"),
> > +	DEFINE_RES_MEM_NAMED(APL_GPIO_SOUTHWEST_OFFSET,
> > +		APL_GPIO_SOUTHWEST_NPIN * SZ_8, "apl_pinctrl_sw"),
> > +	DEFINE_RES_IRQ(APL_GPIO_IRQ),
> > +};
> > +
> > +static struct pinctrl_pin_desc apl_pinctrl_pdata;
> 
> This seems pretty pointless.  What's it for?
> 
Since we do not need to access platform data at here, I have removed
the struct, .pdata_size and .platform_data (from below mfd_cell).
> > +static struct mfd_cell apl_gpio_devices[] = {
> > +	{
> > +		.name = "apl-pinctrl",
> > +		.id = 0,
> > +		.num_resources = ARRAY_SIZE(apl_gpio_io_res),
> > +		.resources = apl_gpio_io_res,
> > +		.pdata_size = sizeof(apl_pinctrl_pdata),
> > +		.platform_data = &apl_pinctrl_pdata,
> > +		.ignore_resource_conflicts = true,
> > +	},
> > +	{
> > +		.name = "apl-pinctrl",
> > +		.id = 1,
> > +		.num_resources = ARRAY_SIZE(apl_gpio_io_res),
> > +		.resources = apl_gpio_io_res,
> > +		.pdata_size = sizeof(apl_pinctrl_pdata),
> > +		.platform_data = &apl_pinctrl_pdata,
> > +		.ignore_resource_conflicts = true,
> > +	},
> > +	{
> > +		.name = "apl-pinctrl",
> > +		.id = 2,
> > +		.num_resources = ARRAY_SIZE(apl_gpio_io_res),
> > +		.resources = apl_gpio_io_res,
> > +		.pdata_size = sizeof(apl_pinctrl_pdata),
> > +		.platform_data = &apl_pinctrl_pdata,
> > +		.ignore_resource_conflicts = true,
> > +	},
> > +	{
> > +		.name = "apl-pinctrl",
> > +		.id = 3,
> > +		.num_resources = ARRAY_SIZE(apl_gpio_io_res),
> > +		.resources = apl_gpio_io_res,
> > +		.pdata_size = sizeof(apl_pinctrl_pdata),
> > +		.platform_data = &apl_pinctrl_pdata,
> > +		.ignore_resource_conflicts = true,
> > +	},
> > +};
> > +
> > +int lpc_ich_add_gpio(struct pci_dev *dev, enum lpc_chipsets chipset)
> > +{
> > +	unsigned int apl_p2sb = PCI_DEVFN(PCI_IDSEL_P2SB, 0);
> 
> You only use this variable one.  Just switch it out for the macro.
> 
Noted. The changes will be applied in next patch-set.
> > +	unsigned int i;
> > +	int ret;
> > +
> > +	if (chipset != LPC_APL)
> > +		return -ENODEV;
> > +	/*
> > +	 * Apollo lake, has not 1, but 4 gpio controllers,
> > +	 * handle it a bit differently.
> > +	 */
> > +
> > +	for (i = 0; i < ARRAY_SIZE(apl_gpio_io_res)-1; i++) {
> 
> This is fragile.  Just define a GPIO_CONTROLLER_COUNT or similar.
> 
Okay.
> > +		struct resource *res = &apl_gpio_io_res[i];
> > +
> > +		apl_gpio_devices[i].resources = res;
> 
> What's happening here?  Are you manually pulling resources out of the
> resource structure and linking them to the device cells?
> 
> Why?
> 
There are total 4 GPIO controllers / communities and each of them has
different MMIO offset addresses. I have removed following line
	apl_gpio_devices[i].resources = res;
in my next patch-set and make modification at device cell, something
like below:
	static struct mfd_cell apl_gpio_devices[] = {
		{
			.name = "apl-pinctrl",
			.id = 0,
			.num_resources = ARRAY_SIZE(apl_gpio_io_res),
			.resources = &apl_gpio_io_res[0],
			.ignore_resource_conflicts = true,
		},
	...
> > +		/* Fill MEM resource */
> > +		ret = p2sb_bar(dev, apl_p2sb, res++);
> 
> Why res++?
> 
I have simplified it so that no need to call p2sb_bar() function four times
inside the for loop. And make p2sb_bar() function just to fill in the base
address into a scratch "struct resource" and have the loop do the additions
to base/end. Something like:
	struct resource base;
	
	ret = p2sb_bar(dev, PCI_DEVFN(PCI_IDSEL_P2SB, 0), &base);
	
	for (i = 0; i < ARRAY_SIZE(GPIO_CONTROLLER_COUNT); i++) {
		struct resource *res = apl_gpio_io_res[i];
		
		apl_gpio_devices[i].resources = res;
		
		/* Fill MEM resource */
		res->start += base.start;
		res->end += base.start;
		res->flags = base.flags;

		res++;
		...
> > +		if (ret)
> > +			goto warn_continue;
> > +
> > +		apl_pinctrl_pdata.name = kasprintf(GFP_KERNEL, "%u",
> > +			i + 1);
> 
> Are you sure this does what you think it does?
> 
> 
> I think you will keep changing *.name, and the only one you will see/use is
> the last one.
> 
> Have you tested this?
> 
> > +		if (apl_pinctrl_pdata.name == NULL) {
> > +			ret = -ENOMEM;
> > +			goto warn_continue;
> 
> No, this is a failure.  You need to 'error' out and return ret.
> 
The entire apl_pinctrl_pdata.name memory allocation is no longer needed here
and tested working fine after remove.
> > +		}
> > +	}
> > +
> > +	ret = mfd_add_devices(&dev->dev, 0,
> > +		apl_gpio_devices, ARRAY_SIZE(apl_gpio_devices),
> > +			NULL, 0, NULL);
> > +
> > +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);
> 
> You've just passed the child device a NULL pointer.
> 
> > +	return 0;
> 
> Why are you returning 0 even during failure?
> 
It should be return ret. 
> > +}
> > diff --git a/drivers/mfd/lpc_ich-apl.h b/drivers/mfd/lpc_ich-apl.h new
> > file mode 100644 index 0000000..db8325d
> > --- /dev/null
> > +++ b/drivers/mfd/lpc_ich-apl.h
> > @@ -0,0 +1,28 @@
> > +/*
> > + * lpc_ich-apl.h - Intel in-vehicle infotainment (IVI) systems used in cars
> > + *                 support
> 
> The "In", "Vehicle" and "Infotainment" should be capitalised.
> 
Noted. The changes will be applied in next patch-set. 
> > + * Copyright (C) 2016, Intel Corporation
> > + *
> > + * Authors: Tan, Jui Nee <jui.nee.tan@intel.com>
> 
> "Author"
> 
Thanks for pointing that out. The changes will be applied in next patch-set.
> > + * This program is free software; you can redistribute it and/or
> > + modify
> > + * it under the terms of the GNU General Public License version 2 as
> > + * published by the Free Software Foundation.
> > + */
> > +
> > +#ifndef __LPC_ICH_APL_H__
> > +#define __LPC_ICH_APL_H__
> > +
> > +#include <linux/pci.h>
> > +
> > +#if IS_ENABLED(CONFIG_X86_INTEL_IVI)
> > +int lpc_ich_add_gpio(struct pci_dev *dev, enum lpc_chipsets chipset);
> > +#else /* CONFIG_X86_INTEL_IVI is not set */ static inline int
> > +lpc_ich_add_gpio(struct pci_dev *dev, enum lpc_chipsets chipset) {
> > +	return -ENODEV;
> > +}
> > +#endif
> > +
> > +#endif
> > diff --git a/drivers/mfd/lpc_ich-core.c b/drivers/mfd/lpc_ich-core.c
> > index bd3aa45..e09d1e2 100644
> > --- a/drivers/mfd/lpc_ich-core.c
> > +++ b/drivers/mfd/lpc_ich-core.c
> > @@ -69,6 +69,8 @@
> >  #include <linux/mfd/lpc_ich.h>
> >  #include <linux/platform_data/itco_wdt.h>
> >
> > +#include "lpc_ich-apl.h"
> > +
> >  #define ACPIBASE		0x40
> >  #define ACPIBASE_GPE_OFF	0x28
> >  #define ACPIBASE_GPE_END	0x2f
> > @@ -147,77 +149,6 @@ static struct mfd_cell lpc_ich_gpio_cell = {
> >  	.ignore_resource_conflicts = true,
> >  };
> >
> > -/* chipset related info */
> > -enum lpc_chipsets {
> > -	LPC_ICH = 0,	/* ICH */
> > -	LPC_ICH0,	/* ICH0 */
> > -	LPC_ICH2,	/* ICH2 */
> > -	LPC_ICH2M,	/* ICH2-M */
> > -	LPC_ICH3,	/* ICH3-S */
> > -	LPC_ICH3M,	/* ICH3-M */
> > -	LPC_ICH4,	/* ICH4 */
> > -	LPC_ICH4M,	/* ICH4-M */
> > -	LPC_CICH,	/* C-ICH */
> > -	LPC_ICH5,	/* ICH5 & ICH5R */
> > -	LPC_6300ESB,	/* 6300ESB */
> > -	LPC_ICH6,	/* ICH6 & ICH6R */
> > -	LPC_ICH6M,	/* ICH6-M */
> > -	LPC_ICH6W,	/* ICH6W & ICH6RW */
> > -	LPC_631XESB,	/* 631xESB/632xESB */
> > -	LPC_ICH7,	/* ICH7 & ICH7R */
> > -	LPC_ICH7DH,	/* ICH7DH */
> > -	LPC_ICH7M,	/* ICH7-M & ICH7-U */
> > -	LPC_ICH7MDH,	/* ICH7-M DH */
> > -	LPC_NM10,	/* NM10 */
> > -	LPC_ICH8,	/* ICH8 & ICH8R */
> > -	LPC_ICH8DH,	/* ICH8DH */
> > -	LPC_ICH8DO,	/* ICH8DO */
> > -	LPC_ICH8M,	/* ICH8M */
> > -	LPC_ICH8ME,	/* ICH8M-E */
> > -	LPC_ICH9,	/* ICH9 */
> > -	LPC_ICH9R,	/* ICH9R */
> > -	LPC_ICH9DH,	/* ICH9DH */
> > -	LPC_ICH9DO,	/* ICH9DO */
> > -	LPC_ICH9M,	/* ICH9M */
> > -	LPC_ICH9ME,	/* ICH9M-E */
> > -	LPC_ICH10,	/* ICH10 */
> > -	LPC_ICH10R,	/* ICH10R */
> > -	LPC_ICH10D,	/* ICH10D */
> > -	LPC_ICH10DO,	/* ICH10DO */
> > -	LPC_PCH,	/* PCH Desktop Full Featured */
> > -	LPC_PCHM,	/* PCH Mobile Full Featured */
> > -	LPC_P55,	/* P55 */
> > -	LPC_PM55,	/* PM55 */
> > -	LPC_H55,	/* H55 */
> > -	LPC_QM57,	/* QM57 */
> > -	LPC_H57,	/* H57 */
> > -	LPC_HM55,	/* HM55 */
> > -	LPC_Q57,	/* Q57 */
> > -	LPC_HM57,	/* HM57 */
> > -	LPC_PCHMSFF,	/* PCH Mobile SFF Full Featured */
> > -	LPC_QS57,	/* QS57 */
> > -	LPC_3400,	/* 3400 */
> > -	LPC_3420,	/* 3420 */
> > -	LPC_3450,	/* 3450 */
> > -	LPC_EP80579,	/* EP80579 */
> > -	LPC_CPT,	/* Cougar Point */
> > -	LPC_CPTD,	/* Cougar Point Desktop */
> > -	LPC_CPTM,	/* Cougar Point Mobile */
> > -	LPC_PBG,	/* Patsburg */
> > -	LPC_DH89XXCC,	/* DH89xxCC */
> > -	LPC_PPT,	/* Panther Point */
> > -	LPC_LPT,	/* Lynx Point */
> > -	LPC_LPT_LP,	/* Lynx Point-LP */
> > -	LPC_WBG,	/* Wellsburg */
> > -	LPC_AVN,	/* Avoton SoC */
> > -	LPC_BAYTRAIL,   /* Bay Trail SoC */
> > -	LPC_COLETO,	/* Coleto Creek */
> > -	LPC_WPT_LP,	/* Wildcat Point-LP */
> > -	LPC_BRASWELL,	/* Braswell SoC */
> > -	LPC_LEWISBURG,	/* Lewisburg */
> > -	LPC_9S,		/* 9 Series */
> > -};
> 
> What does this move have to do with this patch?
> 
> I think it should be separate and be paired with a reason for the change.
> 
Noted. I will place that in separate patch.
> >  static struct lpc_ich_info lpc_chipset_info[] = {
> >  	[LPC_ICH] = {
> >  		.name = "ICH",
> > @@ -531,6 +462,10 @@ static struct lpc_ich_info lpc_chipset_info[] = {
> >  		.name = "9 Series",
> >  		.iTCO_version = 2,
> >  	},
> > +	[LPC_APL]  = {
> > +		.name = "Apollo Lake SoC",
> > +		.iTCO_version = 5,
> > +	},
> 
> Enabling a new platform should be a separate patch.
> 
Okay. I will place that in separate patch.
> >  };
> >
> >  /*
> > @@ -679,6 +614,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}, @@ -1093,6 +1029,9 @@
> static
> > int lpc_ich_probe(struct pci_dev *dev,
> >  			cell_added = true;
> >  	}
> >
> > +	if (!lpc_ich_add_gpio(dev, priv->chipset))
> > +		cell_added = true;
> > +
> >  	/*
> >  	 * We only care if at least one or none of the cells registered
> >  	 * successfully.
> > diff --git a/include/linux/mfd/lpc_ich.h b/include/linux/mfd/lpc_ich.h
> > index 2b300b4..03c2ca3 100644
> > --- a/include/linux/mfd/lpc_ich.h
> > +++ b/include/linux/mfd/lpc_ich.h
> > @@ -34,6 +34,7 @@ enum {
> >  	ICH_V10CORP_GPIO,
> >  	ICH_V10CONS_GPIO,
> >  	AVOTON_GPIO,
> > +	APL_GPIO,
> >  };
> >
> >  struct lpc_ich_info {
> > @@ -43,4 +44,76 @@ struct lpc_ich_info {
> >  	u8 use_gpio;
> >  };
> >
> > +/* chipset related info */
> > +enum lpc_chipsets {
> > +	LPC_ICH = 0,	/* ICH */
> > +	LPC_ICH0,	/* ICH0 */
> > +	LPC_ICH2,	/* ICH2 */
> > +	LPC_ICH2M,	/* ICH2-M */
> > +	LPC_ICH3,	/* ICH3-S */
> > +	LPC_ICH3M,	/* ICH3-M */
> > +	LPC_ICH4,	/* ICH4 */
> > +	LPC_ICH4M,	/* ICH4-M */
> > +	LPC_CICH,	/* C-ICH */
> > +	LPC_ICH5,	/* ICH5 & ICH5R */
> > +	LPC_6300ESB,	/* 6300ESB */
> > +	LPC_ICH6,	/* ICH6 & ICH6R */
> > +	LPC_ICH6M,	/* ICH6-M */
> > +	LPC_ICH6W,	/* ICH6W & ICH6RW */
> > +	LPC_631XESB,	/* 631xESB/632xESB */
> > +	LPC_ICH7,	/* ICH7 & ICH7R */
> > +	LPC_ICH7DH,	/* ICH7DH */
> > +	LPC_ICH7M,	/* ICH7-M & ICH7-U */
> > +	LPC_ICH7MDH,	/* ICH7-M DH */
> > +	LPC_NM10,	/* NM10 */
> > +	LPC_ICH8,	/* ICH8 & ICH8R */
> > +	LPC_ICH8DH,	/* ICH8DH */
> > +	LPC_ICH8DO,	/* ICH8DO */
> > +	LPC_ICH8M,	/* ICH8M */
> > +	LPC_ICH8ME,	/* ICH8M-E */
> > +	LPC_ICH9,	/* ICH9 */
> > +	LPC_ICH9R,	/* ICH9R */
> > +	LPC_ICH9DH,	/* ICH9DH */
> > +	LPC_ICH9DO,	/* ICH9DO */
> > +	LPC_ICH9M,	/* ICH9M */
> > +	LPC_ICH9ME,	/* ICH9M-E */
> > +	LPC_ICH10,	/* ICH10 */
> > +	LPC_ICH10R,	/* ICH10R */
> > +	LPC_ICH10D,	/* ICH10D */
> > +	LPC_ICH10DO,	/* ICH10DO */
> > +	LPC_PCH,	/* PCH Desktop Full Featured */
> > +	LPC_PCHM,	/* PCH Mobile Full Featured */
> > +	LPC_P55,	/* P55 */
> > +	LPC_PM55,	/* PM55 */
> > +	LPC_H55,	/* H55 */
> > +	LPC_QM57,	/* QM57 */
> > +	LPC_H57,	/* H57 */
> > +	LPC_HM55,	/* HM55 */
> > +	LPC_Q57,	/* Q57 */
> > +	LPC_HM57,	/* HM57 */
> > +	LPC_PCHMSFF,	/* PCH Mobile SFF Full Featured */
> > +	LPC_QS57,	/* QS57 */
> > +	LPC_3400,	/* 3400 */
> > +	LPC_3420,	/* 3420 */
> > +	LPC_3450,	/* 3450 */
> > +	LPC_EP80579,	/* EP80579 */
> > +	LPC_CPT,	/* Cougar Point */
> > +	LPC_CPTD,	/* Cougar Point Desktop */
> > +	LPC_CPTM,	/* Cougar Point Mobile */
> > +	LPC_PBG,	/* Patsburg */
> > +	LPC_DH89XXCC,	/* DH89xxCC */
> > +	LPC_PPT,	/* Panther Point */
> > +	LPC_LPT,	/* Lynx Point */
> > +	LPC_LPT_LP,	/* Lynx Point-LP */
> > +	LPC_WBG,	/* Wellsburg */
> > +	LPC_AVN,	/* Avoton SoC */
> > +	LPC_BAYTRAIL,   /* Bay Trail SoC */
> > +	LPC_COLETO,	/* Coleto Creek */
> > +	LPC_WPT_LP,	/* Wildcat Point-LP */
> > +	LPC_BRASWELL,	/* Braswell SoC */
> > +	LPC_LEWISBURG,	/* Lewisburg */
> > +	LPC_9S,		/* 9 Series */
> > +	LPC_APL,	/* Apollo Lake SoC */
> > +};
> > +
> >  #endif
> 
> --
> Lee Jones
> Linaro STMicroelectronics Landing Team Lead Linaro.org │ Open source
> software for ARM SoCs Follow Linaro: Facebook | Twitter | Blog

RE: [PATCH v6 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: Tuesday, August 9, 2016 3:16 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;
> linus.walleij@linaro.org; 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 v6 3/3] mfd: lpc_ich: Add support for Intel Apollo Lake
> GPIO pinctrl in non-ACPI system
> 
> On Thu, 14 Jul 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>
> > ---
> > Changes in V6:
> > 	- Rename CONFIG_X86_INTEL_APL to CONFIG_X86_INTEL_IVI so
> that it
> > 	  relates to the actual product, as suggested by Mika.
> > 	- Rework Makefile according Andy's comments.
> > 	- Rename lpc_ich_misc() to lpc_ich_add_gpio() so that the name
> should not
> > 	  be so generic, as suggested by Andy.
> > 	- Call lpc_ich_add_gpio() via priv->chipset.
> > 	- lpc_ich_add_gpio() function will be moved from
> > 	  .../include/linux/mfd/lpc_ich.h to
> > 	  .../drivers/mfd/lpc_ich-apl.h
> > 	  as this is a part of internal driver interface as suggested by Andy.
> > 	- Move enum lpc_chipsets from
> > 	  .../drivers/mfd/lpc_ich-core.c to
> > 	  .../include/linux/mfd/lpc_ich.h
> > 	  as lpc_chipsets is also accessed by lpc_ich_add_gpio().
> > 	- Check if kasprintf return value for all 4 gpio controllers before
> > 	  proceed to add platform device by using mfd_add_devices().
> >
> > Changes in V5:
> > 	- Split lpc-ich driver into two parts (lpc_ich-core and lpc_ich-apl).
> > 	  The file lpc_ich-apl.c introduces gpio platform driver in MFD.
> > 	- Rename Kconfig option CONFIG_X86_INTEL_NON_ACPI to
> CONFIG_X86_INTEL_APL
> > 	  so that it reflects actual product as suggested by Mika.
> >
> > Changes in V4:
> > 	- Move Kconfig option CONFIG_X86_INTEL_NON_ACPI from
> > 	  [PATCH 2/3] x86/platform/p2sb: New Primary to Sideband bridge
> support driver for Intel SOC's
> > 	  to
> > 	  [PATCH 3/3] mfd: lpc_ich: Add support for Intel Apollo Lake GPIO
> pinctrl in non-ACPI system
> > 	  since the config is used in latter patch.
> > 	- Select CONFIG_P2SB when CONFIG_LPC_ICH is enabled.
> > 	- Remove #ifdef CONFIG_X86_INTEL_NON_ACPI and use
> > 	  #if defined(CONFIG_X86_INTEL_NON_ACPI) when lpc_ich_misc is
> called
> > 	  as suggested by Lee Jones.
> > 	- Use single dimensional array instead of 2D array for apl_gpio_io_res
> > 	  structure and use DEFINE_RES_IRQ for its IRQ resource.
> >
> > Changes in V3:
> > 	- Simplify register addresses calculation and use
> DEFINE_RES_MEM_NAMED
> > 	  defines for apl_gpio_io_res structure
> > 	- Define magic number for P2SB PCI ID
> > 	- Replace switch-case with if-else since currently we have only one
> > 	  use case
> > 	- Only call mfd_add_devices() once for all gpio communities
> >
> > Changes in V2:
> > 	- Add new config option CONFIG_X86_INTEL_NON_ACPI and "select
> PINCTRL"
> > 	  to fix kbuildbot error
> >
> >  arch/x86/Kconfig            |   9 +++
> >  drivers/mfd/Kconfig         |   3 +-
> >  drivers/mfd/Makefile        |   5 +-
> >  drivers/mfd/lpc_ich-apl.c   | 130
> ++++++++++++++++++++++++++++++++++++++++++++
> >  drivers/mfd/lpc_ich-apl.h   |  28 ++++++++++
> >  drivers/mfd/lpc_ich-core.c  |  81 ++++-----------------------
> > include/linux/mfd/lpc_ich.h |  73 +++++++++++++++++++++++++
> >  7 files changed, 256 insertions(+), 73 deletions(-)  create mode
> > 100644 drivers/mfd/lpc_ich-apl.c  create mode 100644
> > drivers/mfd/lpc_ich-apl.h
> >
> > diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig index
> > d305d81..c0b427b 100644
> > --- a/arch/x86/Kconfig
> > +++ b/arch/x86/Kconfig
> > @@ -513,6 +513,15 @@ config X86_INTEL_CE
> >  	  This option compiles in support for the CE4100 SOC for settop
> >  	  boxes and media devices.
> >
> > +config X86_INTEL_IVI
> > +	bool "Intel in-vehicle infotainment (IVI) systems used in cars"
> > +	select PINCTRL
> > +	---help---
> > +	  Select this option to enable MMIO BAR access over the P2SB for
> > +	  non-ACPI Intel Apollo Lake SoC platforms. This driver uses the P2SB
> > +	  hide/unhide mechanism cooperatively to pass the PCI BAR address
> to
> > +	  the platform driver, currently GPIO.
> > +
> 
> This should be a separate patch.
> 
Thanks for your comment. The changes will be applied in next patch-set.
> >  config X86_INTEL_MID
> >  	bool "Intel MID platform support"
> >  	depends on X86_EXTENDED_PLATFORM
> > diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig index
> > 1bcf601..dc4e543 100644
> > --- a/drivers/mfd/Kconfig
> > +++ b/drivers/mfd/Kconfig
> > @@ -369,8 +369,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
> >  	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/Makefile b/drivers/mfd/Makefile index
> > 1dfe5fb..0aa3e1f 100644
> > --- a/drivers/mfd/Makefile
> > +++ b/drivers/mfd/Makefile
> > @@ -155,8 +155,11 @@ obj-$(CONFIG_PMIC_ADP5520)	+= adp5520.o
> >  obj-$(CONFIG_MFD_KEMPLD)	+= kempld-core.o
> >  obj-$(CONFIG_MFD_INTEL_QUARK_I2C_GPIO)	+=
> intel_quark_i2c_gpio.o
> >  obj-$(CONFIG_LPC_SCH)		+= lpc_sch.o
> > -lpc_ich-objs		:= lpc_ich-core.o
> >  obj-$(CONFIG_LPC_ICH)		+= lpc_ich.o
> > +lpc_ich-objs		:= lpc_ich-core.o
> > +ifeq ($(CONFIG_X86_INTEL_IVI),y)
> > +lpc_ich-objs += lpc_ich-apl.o
> > +endif
> >  obj-$(CONFIG_MFD_RDC321X)	+= rdc321x-southbridge.o
> >  obj-$(CONFIG_MFD_JANZ_CMODIO)	+= janz-cmodio.o
> >  obj-$(CONFIG_MFD_JZ4740_ADC)	+= jz4740-adc.o
> > diff --git a/drivers/mfd/lpc_ich-apl.c b/drivers/mfd/lpc_ich-apl.c new
> > file mode 100644 index 0000000..e4b9688
> > --- /dev/null
> > +++ b/drivers/mfd/lpc_ich-apl.c
> > @@ -0,0 +1,130 @@
> > +/*
> > + * Purpose: Create a platform device to bind with Intel Apollo Lake
> 
> Never seen this before.  Looks like more of a commit log entry than
> something that should live in a source file header.
> 
> > + * Pinctrl GPIO platform driver
> 
> No need to mention "platform driver" here. Most drivers are.
> 
Okay, the changes will be applied in next patch-set.
> > + * Copyright (C) 2016 Intel Corporation
> 
> Author?
> 
I will add the Author information in next patch-set.
> > + * This program is free software; you can redistribute it and/or
> > + modify
> > + * it under the terms of the GNU General Public License version 2 as
> > + * published by the Free Software Foundation.
> > + */
> > +
> > +#include <linux/pci.h>
> > +#include <linux/mfd/core.h>
> > +#include <linux/mfd/lpc_ich.h>
> > +#include <linux/pinctrl/pinctrl.h>
> > +#include <asm/p2sb.h>
> 
> Alphabetical.
> 
Thanks for pointing that out.
> > +#include "lpc_ich-apl.h"
> 
> Don't' mix '_'s with '-'s.
> 
The header file will be renamed as lpc_ich_apl.h.
> > +/* Offset data for Apollo Lake GPIO communities */
> > +#define APL_GPIO_SOUTHWEST_OFFSET	0xc00000
> > +#define APL_GPIO_NORTHWEST_OFFSET	0xc40000
> > +#define APL_GPIO_NORTH_OFFSET		0xc50000
> > +#define APL_GPIO_WEST_OFFSET		0xc70000
> > +
> > +#define APL_GPIO_SOUTHWEST_NPIN		43
> > +#define APL_GPIO_NORTHWEST_NPIN		77
> > +#define APL_GPIO_NORTH_NPIN		78
> > +#define APL_GPIO_WEST_NPIN		47
> > +
> > +#define APL_GPIO_IRQ 14
> > +
> > +#define PCI_IDSEL_P2SB	0x0d
> > +
> > +static struct resource apl_gpio_io_res[] = {
> > +	DEFINE_RES_MEM_NAMED(APL_GPIO_NORTH_OFFSET,
> > +		APL_GPIO_NORTH_NPIN * SZ_8, "apl_pinctrl_n"),
> > +	DEFINE_RES_MEM_NAMED(APL_GPIO_NORTHWEST_OFFSET,
> > +		APL_GPIO_NORTHWEST_NPIN * SZ_8, "apl_pinctrl_nw"),
> > +	DEFINE_RES_MEM_NAMED(APL_GPIO_WEST_OFFSET,
> > +		APL_GPIO_WEST_NPIN * SZ_8, "apl_pinctrl_w"),
> > +	DEFINE_RES_MEM_NAMED(APL_GPIO_SOUTHWEST_OFFSET,
> > +		APL_GPIO_SOUTHWEST_NPIN * SZ_8, "apl_pinctrl_sw"),
> > +	DEFINE_RES_IRQ(APL_GPIO_IRQ),
> > +};
> > +
> > +static struct pinctrl_pin_desc apl_pinctrl_pdata;
> 
> This seems pretty pointless.  What's it for?
> 
Since we do not need to access platform data at here, I have removed
the struct, .pdata_size and .platform_data (from below mfd_cell).
> > +static struct mfd_cell apl_gpio_devices[] = {
> > +	{
> > +		.name = "apl-pinctrl",
> > +		.id = 0,
> > +		.num_resources = ARRAY_SIZE(apl_gpio_io_res),
> > +		.resources = apl_gpio_io_res,
> > +		.pdata_size = sizeof(apl_pinctrl_pdata),
> > +		.platform_data = &apl_pinctrl_pdata,
> > +		.ignore_resource_conflicts = true,
> > +	},
> > +	{
> > +		.name = "apl-pinctrl",
> > +		.id = 1,
> > +		.num_resources = ARRAY_SIZE(apl_gpio_io_res),
> > +		.resources = apl_gpio_io_res,
> > +		.pdata_size = sizeof(apl_pinctrl_pdata),
> > +		.platform_data = &apl_pinctrl_pdata,
> > +		.ignore_resource_conflicts = true,
> > +	},
> > +	{
> > +		.name = "apl-pinctrl",
> > +		.id = 2,
> > +		.num_resources = ARRAY_SIZE(apl_gpio_io_res),
> > +		.resources = apl_gpio_io_res,
> > +		.pdata_size = sizeof(apl_pinctrl_pdata),
> > +		.platform_data = &apl_pinctrl_pdata,
> > +		.ignore_resource_conflicts = true,
> > +	},
> > +	{
> > +		.name = "apl-pinctrl",
> > +		.id = 3,
> > +		.num_resources = ARRAY_SIZE(apl_gpio_io_res),
> > +		.resources = apl_gpio_io_res,
> > +		.pdata_size = sizeof(apl_pinctrl_pdata),
> > +		.platform_data = &apl_pinctrl_pdata,
> > +		.ignore_resource_conflicts = true,
> > +	},
> > +};
> > +
> > +int lpc_ich_add_gpio(struct pci_dev *dev, enum lpc_chipsets chipset)
> > +{
> > +	unsigned int apl_p2sb = PCI_DEVFN(PCI_IDSEL_P2SB, 0);
> 
> You only use this variable one.  Just switch it out for the macro.
> 
Noted. The changes will be applied in next patch-set.
> > +	unsigned int i;
> > +	int ret;
> > +
> > +	if (chipset != LPC_APL)
> > +		return -ENODEV;
> > +	/*
> > +	 * Apollo lake, has not 1, but 4 gpio controllers,
> > +	 * handle it a bit differently.
> > +	 */
> > +
> > +	for (i = 0; i < ARRAY_SIZE(apl_gpio_io_res)-1; i++) {
> 
> This is fragile.  Just define a GPIO_CONTROLLER_COUNT or similar.
> 
Okay.
> > +		struct resource *res = &apl_gpio_io_res[i];
> > +
> > +		apl_gpio_devices[i].resources = res;
> 
> What's happening here?  Are you manually pulling resources out of the
> resource structure and linking them to the device cells?
> 
> Why?
> 
There are total 4 GPIO controllers / communities and each of them has
different MMIO offset addresses. I have removed following line
	apl_gpio_devices[i].resources = res;
in my next patch-set and make modification at device cell, something
like below:
	static struct mfd_cell apl_gpio_devices[] = {
		{
			.name = "apl-pinctrl",
			.id = 0,
			.num_resources = ARRAY_SIZE(apl_gpio_io_res),
			.resources = &apl_gpio_io_res[0],
			.ignore_resource_conflicts = true,
		},
	...
> > +		/* Fill MEM resource */
> > +		ret = p2sb_bar(dev, apl_p2sb, res++);
> 
> Why res++?
> 
I have simplified it so that no need to call p2sb_bar() function four times
inside the for loop. And make p2sb_bar() function just to fill in the base
address into a scratch "struct resource" and have the loop do the additions
to base/end. Something like:
	struct resource base;
	
	ret = p2sb_bar(dev, PCI_DEVFN(PCI_IDSEL_P2SB, 0), &base);
	
	for (i = 0; i < ARRAY_SIZE(GPIO_CONTROLLER_COUNT); i++) {
		struct resource *res = apl_gpio_io_res[i];
		
		apl_gpio_devices[i].resources = res;
		
		/* Fill MEM resource */
		res->start += base.start;
		res->end += base.start;
		res->flags = base.flags;

		res++;
		...
> > +		if (ret)
> > +			goto warn_continue;
> > +
> > +		apl_pinctrl_pdata.name = kasprintf(GFP_KERNEL, "%u",
> > +			i + 1);
> 
> Are you sure this does what you think it does?
> 
> 
> I think you will keep changing *.name, and the only one you will see/use is
> the last one.
> 
> Have you tested this?
> 
> > +		if (apl_pinctrl_pdata.name == NULL) {
> > +			ret = -ENOMEM;
> > +			goto warn_continue;
> 
> No, this is a failure.  You need to 'error' out and return ret.
> 
The entire apl_pinctrl_pdata.name memory allocation is no longer needed here
and tested working fine after remove.
> > +		}
> > +	}
> > +
> > +	ret = mfd_add_devices(&dev->dev, 0,
> > +		apl_gpio_devices, ARRAY_SIZE(apl_gpio_devices),
> > +			NULL, 0, NULL);
> > +
> > +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);
> 
> You've just passed the child device a NULL pointer.
> 
> > +	return 0;
> 
> Why are you returning 0 even during failure?
> 
It should be return ret. 
> > +}
> > diff --git a/drivers/mfd/lpc_ich-apl.h b/drivers/mfd/lpc_ich-apl.h new
> > file mode 100644 index 0000000..db8325d
> > --- /dev/null
> > +++ b/drivers/mfd/lpc_ich-apl.h
> > @@ -0,0 +1,28 @@
> > +/*
> > + * lpc_ich-apl.h - Intel in-vehicle infotainment (IVI) systems used in cars
> > + *                 support
> 
> The "In", "Vehicle" and "Infotainment" should be capitalised.
> 
Noted. The changes will be applied in next patch-set. 
> > + * Copyright (C) 2016, Intel Corporation
> > + *
> > + * Authors: Tan, Jui Nee <jui.nee.tan@intel.com>
> 
> "Author"
> 
Thanks for pointing that out. The changes will be applied in next patch-set.
> > + * This program is free software; you can redistribute it and/or
> > + modify
> > + * it under the terms of the GNU General Public License version 2 as
> > + * published by the Free Software Foundation.
> > + */
> > +
> > +#ifndef __LPC_ICH_APL_H__
> > +#define __LPC_ICH_APL_H__
> > +
> > +#include <linux/pci.h>
> > +
> > +#if IS_ENABLED(CONFIG_X86_INTEL_IVI)
> > +int lpc_ich_add_gpio(struct pci_dev *dev, enum lpc_chipsets chipset);
> > +#else /* CONFIG_X86_INTEL_IVI is not set */ static inline int
> > +lpc_ich_add_gpio(struct pci_dev *dev, enum lpc_chipsets chipset) {
> > +	return -ENODEV;
> > +}
> > +#endif
> > +
> > +#endif
> > diff --git a/drivers/mfd/lpc_ich-core.c b/drivers/mfd/lpc_ich-core.c
> > index bd3aa45..e09d1e2 100644
> > --- a/drivers/mfd/lpc_ich-core.c
> > +++ b/drivers/mfd/lpc_ich-core.c
> > @@ -69,6 +69,8 @@
> >  #include <linux/mfd/lpc_ich.h>
> >  #include <linux/platform_data/itco_wdt.h>
> >
> > +#include "lpc_ich-apl.h"
> > +
> >  #define ACPIBASE		0x40
> >  #define ACPIBASE_GPE_OFF	0x28
> >  #define ACPIBASE_GPE_END	0x2f
> > @@ -147,77 +149,6 @@ static struct mfd_cell lpc_ich_gpio_cell = {
> >  	.ignore_resource_conflicts = true,
> >  };
> >
> > -/* chipset related info */
> > -enum lpc_chipsets {
> > -	LPC_ICH = 0,	/* ICH */
> > -	LPC_ICH0,	/* ICH0 */
> > -	LPC_ICH2,	/* ICH2 */
> > -	LPC_ICH2M,	/* ICH2-M */
> > -	LPC_ICH3,	/* ICH3-S */
> > -	LPC_ICH3M,	/* ICH3-M */
> > -	LPC_ICH4,	/* ICH4 */
> > -	LPC_ICH4M,	/* ICH4-M */
> > -	LPC_CICH,	/* C-ICH */
> > -	LPC_ICH5,	/* ICH5 & ICH5R */
> > -	LPC_6300ESB,	/* 6300ESB */
> > -	LPC_ICH6,	/* ICH6 & ICH6R */
> > -	LPC_ICH6M,	/* ICH6-M */
> > -	LPC_ICH6W,	/* ICH6W & ICH6RW */
> > -	LPC_631XESB,	/* 631xESB/632xESB */
> > -	LPC_ICH7,	/* ICH7 & ICH7R */
> > -	LPC_ICH7DH,	/* ICH7DH */
> > -	LPC_ICH7M,	/* ICH7-M & ICH7-U */
> > -	LPC_ICH7MDH,	/* ICH7-M DH */
> > -	LPC_NM10,	/* NM10 */
> > -	LPC_ICH8,	/* ICH8 & ICH8R */
> > -	LPC_ICH8DH,	/* ICH8DH */
> > -	LPC_ICH8DO,	/* ICH8DO */
> > -	LPC_ICH8M,	/* ICH8M */
> > -	LPC_ICH8ME,	/* ICH8M-E */
> > -	LPC_ICH9,	/* ICH9 */
> > -	LPC_ICH9R,	/* ICH9R */
> > -	LPC_ICH9DH,	/* ICH9DH */
> > -	LPC_ICH9DO,	/* ICH9DO */
> > -	LPC_ICH9M,	/* ICH9M */
> > -	LPC_ICH9ME,	/* ICH9M-E */
> > -	LPC_ICH10,	/* ICH10 */
> > -	LPC_ICH10R,	/* ICH10R */
> > -	LPC_ICH10D,	/* ICH10D */
> > -	LPC_ICH10DO,	/* ICH10DO */
> > -	LPC_PCH,	/* PCH Desktop Full Featured */
> > -	LPC_PCHM,	/* PCH Mobile Full Featured */
> > -	LPC_P55,	/* P55 */
> > -	LPC_PM55,	/* PM55 */
> > -	LPC_H55,	/* H55 */
> > -	LPC_QM57,	/* QM57 */
> > -	LPC_H57,	/* H57 */
> > -	LPC_HM55,	/* HM55 */
> > -	LPC_Q57,	/* Q57 */
> > -	LPC_HM57,	/* HM57 */
> > -	LPC_PCHMSFF,	/* PCH Mobile SFF Full Featured */
> > -	LPC_QS57,	/* QS57 */
> > -	LPC_3400,	/* 3400 */
> > -	LPC_3420,	/* 3420 */
> > -	LPC_3450,	/* 3450 */
> > -	LPC_EP80579,	/* EP80579 */
> > -	LPC_CPT,	/* Cougar Point */
> > -	LPC_CPTD,	/* Cougar Point Desktop */
> > -	LPC_CPTM,	/* Cougar Point Mobile */
> > -	LPC_PBG,	/* Patsburg */
> > -	LPC_DH89XXCC,	/* DH89xxCC */
> > -	LPC_PPT,	/* Panther Point */
> > -	LPC_LPT,	/* Lynx Point */
> > -	LPC_LPT_LP,	/* Lynx Point-LP */
> > -	LPC_WBG,	/* Wellsburg */
> > -	LPC_AVN,	/* Avoton SoC */
> > -	LPC_BAYTRAIL,   /* Bay Trail SoC */
> > -	LPC_COLETO,	/* Coleto Creek */
> > -	LPC_WPT_LP,	/* Wildcat Point-LP */
> > -	LPC_BRASWELL,	/* Braswell SoC */
> > -	LPC_LEWISBURG,	/* Lewisburg */
> > -	LPC_9S,		/* 9 Series */
> > -};
> 
> What does this move have to do with this patch?
> 
> I think it should be separate and be paired with a reason for the change.
> 
Noted. I will place that in separate patch.
> >  static struct lpc_ich_info lpc_chipset_info[] = {
> >  	[LPC_ICH] = {
> >  		.name = "ICH",
> > @@ -531,6 +462,10 @@ static struct lpc_ich_info lpc_chipset_info[] = {
> >  		.name = "9 Series",
> >  		.iTCO_version = 2,
> >  	},
> > +	[LPC_APL]  = {
> > +		.name = "Apollo Lake SoC",
> > +		.iTCO_version = 5,
> > +	},
> 
> Enabling a new platform should be a separate patch.
> 
Okay. I will place that in separate patch.
> >  };
> >
> >  /*
> > @@ -679,6 +614,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}, @@ -1093,6 +1029,9 @@
> static
> > int lpc_ich_probe(struct pci_dev *dev,
> >  			cell_added = true;
> >  	}
> >
> > +	if (!lpc_ich_add_gpio(dev, priv->chipset))
> > +		cell_added = true;
> > +
> >  	/*
> >  	 * We only care if at least one or none of the cells registered
> >  	 * successfully.
> > diff --git a/include/linux/mfd/lpc_ich.h b/include/linux/mfd/lpc_ich.h
> > index 2b300b4..03c2ca3 100644
> > --- a/include/linux/mfd/lpc_ich.h
> > +++ b/include/linux/mfd/lpc_ich.h
> > @@ -34,6 +34,7 @@ enum {
> >  	ICH_V10CORP_GPIO,
> >  	ICH_V10CONS_GPIO,
> >  	AVOTON_GPIO,
> > +	APL_GPIO,
> >  };
> >
> >  struct lpc_ich_info {
> > @@ -43,4 +44,76 @@ struct lpc_ich_info {
> >  	u8 use_gpio;
> >  };
> >
> > +/* chipset related info */
> > +enum lpc_chipsets {
> > +	LPC_ICH = 0,	/* ICH */
> > +	LPC_ICH0,	/* ICH0 */
> > +	LPC_ICH2,	/* ICH2 */
> > +	LPC_ICH2M,	/* ICH2-M */
> > +	LPC_ICH3,	/* ICH3-S */
> > +	LPC_ICH3M,	/* ICH3-M */
> > +	LPC_ICH4,	/* ICH4 */
> > +	LPC_ICH4M,	/* ICH4-M */
> > +	LPC_CICH,	/* C-ICH */
> > +	LPC_ICH5,	/* ICH5 & ICH5R */
> > +	LPC_6300ESB,	/* 6300ESB */
> > +	LPC_ICH6,	/* ICH6 & ICH6R */
> > +	LPC_ICH6M,	/* ICH6-M */
> > +	LPC_ICH6W,	/* ICH6W & ICH6RW */
> > +	LPC_631XESB,	/* 631xESB/632xESB */
> > +	LPC_ICH7,	/* ICH7 & ICH7R */
> > +	LPC_ICH7DH,	/* ICH7DH */
> > +	LPC_ICH7M,	/* ICH7-M & ICH7-U */
> > +	LPC_ICH7MDH,	/* ICH7-M DH */
> > +	LPC_NM10,	/* NM10 */
> > +	LPC_ICH8,	/* ICH8 & ICH8R */
> > +	LPC_ICH8DH,	/* ICH8DH */
> > +	LPC_ICH8DO,	/* ICH8DO */
> > +	LPC_ICH8M,	/* ICH8M */
> > +	LPC_ICH8ME,	/* ICH8M-E */
> > +	LPC_ICH9,	/* ICH9 */
> > +	LPC_ICH9R,	/* ICH9R */
> > +	LPC_ICH9DH,	/* ICH9DH */
> > +	LPC_ICH9DO,	/* ICH9DO */
> > +	LPC_ICH9M,	/* ICH9M */
> > +	LPC_ICH9ME,	/* ICH9M-E */
> > +	LPC_ICH10,	/* ICH10 */
> > +	LPC_ICH10R,	/* ICH10R */
> > +	LPC_ICH10D,	/* ICH10D */
> > +	LPC_ICH10DO,	/* ICH10DO */
> > +	LPC_PCH,	/* PCH Desktop Full Featured */
> > +	LPC_PCHM,	/* PCH Mobile Full Featured */
> > +	LPC_P55,	/* P55 */
> > +	LPC_PM55,	/* PM55 */
> > +	LPC_H55,	/* H55 */
> > +	LPC_QM57,	/* QM57 */
> > +	LPC_H57,	/* H57 */
> > +	LPC_HM55,	/* HM55 */
> > +	LPC_Q57,	/* Q57 */
> > +	LPC_HM57,	/* HM57 */
> > +	LPC_PCHMSFF,	/* PCH Mobile SFF Full Featured */
> > +	LPC_QS57,	/* QS57 */
> > +	LPC_3400,	/* 3400 */
> > +	LPC_3420,	/* 3420 */
> > +	LPC_3450,	/* 3450 */
> > +	LPC_EP80579,	/* EP80579 */
> > +	LPC_CPT,	/* Cougar Point */
> > +	LPC_CPTD,	/* Cougar Point Desktop */
> > +	LPC_CPTM,	/* Cougar Point Mobile */
> > +	LPC_PBG,	/* Patsburg */
> > +	LPC_DH89XXCC,	/* DH89xxCC */
> > +	LPC_PPT,	/* Panther Point */
> > +	LPC_LPT,	/* Lynx Point */
> > +	LPC_LPT_LP,	/* Lynx Point-LP */
> > +	LPC_WBG,	/* Wellsburg */
> > +	LPC_AVN,	/* Avoton SoC */
> > +	LPC_BAYTRAIL,   /* Bay Trail SoC */
> > +	LPC_COLETO,	/* Coleto Creek */
> > +	LPC_WPT_LP,	/* Wildcat Point-LP */
> > +	LPC_BRASWELL,	/* Braswell SoC */
> > +	LPC_LEWISBURG,	/* Lewisburg */
> > +	LPC_9S,		/* 9 Series */
> > +	LPC_APL,	/* Apollo Lake SoC */
> > +};
> > +
> >  #endif
> 
> --
> Lee Jones
> Linaro STMicroelectronics Landing Team Lead Linaro.org │ Open source
> software for ARM SoCs Follow Linaro: Facebook | Twitter | Blog