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

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

[Tan Jui Nee]

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

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

All these three patches have dependencies on each other.

Changes from V2:
	- 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 from V1:
	- Add new config option CONFIG_X86_INTEL_NON_ACPI and "select PINCTRL"
	  to fix kbuildbot error

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

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

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

-- 
1.9.1

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

[Tan Jui Nee]

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

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

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

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

[Mika Westerberg]

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

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

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

[Linus Walleij]

On Tue, Jun 7, 2016 at 8:55 AM, Tan Jui Nee <jui.nee.tan@intel.com> wrote:

> This is to cater the need for non-ACPI system whereby
> a platform device has to be created in order to bind
> with the Apollo Lake Pinctrl GPIO platform driver.
>
> Signed-off-by: Tan Jui Nee <jui.nee.tan@intel.com>

You forgot to put me on the To: line for the patch so it's only luck that I
saw it. (Every once on a blue moon I read the linux-gpio list directly...)

Patch applied with Mika's ACK.

Yours,
Linus Walleij

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

[Tan, Jui Nee]

> -----Original Message-----
> From: Linus Walleij [mailto:linus.walleij@linaro.org]
> Sent: Tuesday, June 14, 2016 3:09 PM
> To: Tan, Jui Nee <jui.nee.tan@intel.com>
> Cc: Mika Westerberg <mika.westerberg@linux.intel.com>; Heikki Krogerus
> <heikki.krogerus@linux.intel.com>; Andy Shevchenko
> <andriy.shevchenko@linux.intel.com>; Thomas Gleixner
> <tglx@linutronix.de>; Ingo Molnar <mingo@redhat.com>; H. Peter Anvin
> <hpa@zytor.com>; x86@kernel.org; ptyser@xes-inc.com; Lee Jones
> <lee.jones@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 v3 1/3] pinctrl/broxton: enable platform device in the
> absent of ACPI enumeration
> 
> On Tue, Jun 7, 2016 at 8:55 AM, Tan Jui Nee <jui.nee.tan@intel.com> wrote:
> 
> > This is to cater the need for non-ACPI system whereby a platform
> > device has to be created in order to bind with the Apollo Lake Pinctrl
> > GPIO platform driver.
> >
> > Signed-off-by: Tan Jui Nee <jui.nee.tan@intel.com>
> 
> You forgot to put me on the To: line for the patch so it's only luck that I saw it.
> (Every once on a blue moon I read the linux-gpio list directly...)
> 
> Patch applied with Mika's ACK.
> 
> Yours,
> Linus Walleij
Oh, I'm sorry. I will add your name in the subsequent patch/mail. 

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

[Tan Jui Nee]

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

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

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

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

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

[Mika Westerberg]

On Tue, Jun 07, 2016 at 02:55:52PM +0800, Tan Jui Nee 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>
> ---
>  arch/x86/Kconfig                 | 14 ++++++
>  arch/x86/include/asm/p2sb.h      | 27 +++++++++++
>  arch/x86/platform/intel/Makefile |  1 +
>  arch/x86/platform/intel/p2sb.c   | 99 ++++++++++++++++++++++++++++++++++++++++
>  4 files changed, 141 insertions(+)
>  create mode 100644 arch/x86/include/asm/p2sb.h
>  create mode 100644 arch/x86/platform/intel/p2sb.c
> 
> diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
> index 2dc18605..589045e 100644
> --- a/arch/x86/Kconfig
> +++ b/arch/x86/Kconfig
> @@ -606,6 +606,20 @@ config IOSF_MBI_DEBUG
>  
>  	  If you don't require the option or are in doubt, say N.
>  
> +config X86_INTEL_NON_ACPI
> +	bool "Enable support non-ACPI Intel platforms"
> +	select PINCTRL
> +	---help---
> +	  Select this option to enables MMIO BAR access over the P2SB for
> +	  non-ACPI Intel SoC platforms. This driver uses the P2SB hide/unhide
> +	  mechanism cooperatively to pass the PCI BAR address to the platform
> +	  driver, currently GPIO on the following SoC products.
> +	   - Apollo Lake

Why do we need Kconfig option for this?

I think better is to make P2SB available on CPUs which have one, and
that can be detected runtime. If P2SB is not available then p2sb_bar()
returns -ENODEV.

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

[Andy Shevchenko]

On Thu, 2016-06-09 at 17:05 +0300, Mika Westerberg wrote:
> On Tue, Jun 07, 2016 at 02:55:52PM +0800, Tan Jui Nee 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>
> > ---
> >  arch/x86/Kconfig                 | 14 ++++++
> >  arch/x86/include/asm/p2sb.h      | 27 +++++++++++
> >  arch/x86/platform/intel/Makefile |  1 +
> >  arch/x86/platform/intel/p2sb.c   | 99
> > ++++++++++++++++++++++++++++++++++++++++
> >  4 files changed, 141 insertions(+)
> >  create mode 100644 arch/x86/include/asm/p2sb.h
> >  create mode 100644 arch/x86/platform/intel/p2sb.c
> > 
> > diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
> > index 2dc18605..589045e 100644
> > --- a/arch/x86/Kconfig
> > +++ b/arch/x86/Kconfig
> > @@ -606,6 +606,20 @@ config IOSF_MBI_DEBUG
> >  
> >  	  If you don't require the option or are in doubt, say N.
> >  
> > +config X86_INTEL_NON_ACPI
> > +	bool "Enable support non-ACPI Intel platforms"
> > +	select PINCTRL
> > +	---help---
> > +	  Select this option to enables MMIO BAR access over the
> > P2SB for
> > +	  non-ACPI Intel SoC platforms. This driver uses the P2SB
> > hide/unhide
> > +	  mechanism cooperatively to pass the PCI BAR address to
> > the platform
> > +	  driver, currently GPIO on the following SoC products.
> > +	   - Apollo Lake
> 
> Why do we need Kconfig option for this?

In one of previous review I was wondering how we could not to build this
at all. I don't like this option either.

> 
> I think better is to make P2SB available on CPUs which have one, and
> that can be detected runtime. If P2SB is not available then p2sb_bar()
> returns -ENODEV.

Would work to me, though still the same question: is it possible to
avoid building it on even most of Intel platforms, since there, I
assume, will be not many users of the module?

-- 

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

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

[Mika Westerberg]

On Mon, Jun 13, 2016 at 04:54:31PM +0300, Andy Shevchenko wrote:
> Would work to me, though still the same question: is it possible to
> avoid building it on even most of Intel platforms, since there, I
> assume, will be not many users of the module?

Well, even if you make it configurable via Kconfig, I guess distros will
have to enable it in order to support as wide range of CPUs as possible
in a single binary.

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

[Andy Shevchenko]

On Mon, 2016-06-13 at 17:25 +0300, Mika Westerberg wrote:
> On Mon, Jun 13, 2016 at 04:54:31PM +0300, Andy Shevchenko wrote:
> > Would work to me, though still the same question: is it possible to
> > avoid building it on even most of Intel platforms, since there, I
> > assume, will be not many users of the module?
> 
> Well, even if you make it configurable via Kconfig, I guess distros
> will
> have to enable it in order to support as wide range of CPUs as
> possible
> in a single binary.

Good point.

Then perhaps the following we can do:
 - add a static boolean flag
 - add __init function where we check either PCI root bridge ID or CPU
ID (I don't know which one is better, I suppose second one, though it
will require an update of arch/x86/include/asm/intel-family.h)
 - add a check into the function.

What do you think?

-- 

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

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

[Mika Westerberg]

On Mon, Jun 13, 2016 at 06:19:12PM +0300, Andy Shevchenko wrote:
> On Mon, 2016-06-13 at 17:25 +0300, Mika Westerberg wrote:
> > On Mon, Jun 13, 2016 at 04:54:31PM +0300, Andy Shevchenko wrote:
> > > Would work to me, though still the same question: is it possible to
> > > avoid building it on even most of Intel platforms, since there, I
> > > assume, will be not many users of the module?
> > 
> > Well, even if you make it configurable via Kconfig, I guess distros
> > will
> > have to enable it in order to support as wide range of CPUs as
> > possible
> > in a single binary.
> 
> Good point.
> 
> Then perhaps the following we can do:
>  - add a static boolean flag
>  - add __init function where we check either PCI root bridge ID or CPU
> ID (I don't know which one is better, I suppose second one, though it
> will require an update of arch/x86/include/asm/intel-family.h)
>  - add a check into the function.
> 
> What do you think?

Maybe, or make it modular and use MODULE_DEVICE_TABLE(x86cpu, ...) to
match the corresponding CPUs.

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

[Tan, Jui Nee]

> -----Original Message-----
> From: Mika Westerberg [mailto:mika.westerberg@linux.intel.com]
> Sent: Monday, June 13, 2016 11:59 PM
> To: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Cc: Tan, Jui Nee <jui.nee.tan@intel.com>; heikki.krogerus@linux.intel.com;
> tglx@linutronix.de; mingo@redhat.com; hpa@zytor.com; x86@kernel.org;
> ptyser@xes-inc.com; lee.jones@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 v3 2/3] x86/platform/p2sb: New Primary to Sideband
> bridge support driver for Intel SOC's
> 
> On Mon, Jun 13, 2016 at 06:19:12PM +0300, Andy Shevchenko wrote:
> > On Mon, 2016-06-13 at 17:25 +0300, Mika Westerberg wrote:
> > > On Mon, Jun 13, 2016 at 04:54:31PM +0300, Andy Shevchenko wrote:
> > > > Would work to me, though still the same question: is it possible
> > > > to avoid building it on even most of Intel platforms, since there,
> > > > I assume, will be not many users of the module?
> > >
> > > Well, even if you make it configurable via Kconfig, I guess distros
> > > will have to enable it in order to support as wide range of CPUs as
> > > possible in a single binary.
> >
> > Good point.
> >
> > Then perhaps the following we can do:
> >  - add a static boolean flag
> >  - add __init function where we check either PCI root bridge ID or CPU
> > ID (I don't know which one is better, I suppose second one, though it
> > will require an update of arch/x86/include/asm/intel-family.h)
> >  - add a check into the function.
> >
> > What do you think?
> 
> Maybe, or make it modular and use MODULE_DEVICE_TABLE(x86cpu, ...) to
> match the corresponding CPUs.
We need CONFIG_X86_INTEL_NON_ACPI Kconfig option to select CONFIG_PINCTRL.
This is to solve kbuidbot complaint about kernel configuration, i.e.
CONFIG_PINCTRL=n. Appreciate if you could advise something on this.

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

[Mika Westerberg]

On Tue, Jun 21, 2016 at 05:03:20AM +0000, Tan, Jui Nee wrote:
> > Maybe, or make it modular and use MODULE_DEVICE_TABLE(x86cpu, ...) to
> > match the corresponding CPUs.
>
> We need CONFIG_X86_INTEL_NON_ACPI Kconfig option to select CONFIG_PINCTRL.
> This is to solve kbuidbot complaint about kernel configuration, i.e.
> CONFIG_PINCTRL=n. Appreciate if you could advise something on this.

Good point.

Then I guess you might add similar Kconfig option but can you call it
something else than CONFIG_X86_INTEL_NON_ACPI. Perhaps something that
relates to the actual product so distro people can then decide whether
they want to support it or not. Along the lines of CONFIG_X86_INTEL_CE
and so on.

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

[Tan, Jui Nee]

> -----Original Message-----
> From: Mika Westerberg [mailto:mika.westerberg@linux.intel.com]
> Sent: Tuesday, June 21, 2016 3:24 PM
> To: Tan, Jui Nee <jui.nee.tan@intel.com>
> Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>;
> heikki.krogerus@linux.intel.com; tglx@linutronix.de; mingo@redhat.com;
> hpa@zytor.com; x86@kernel.org; ptyser@xes-inc.com;
> lee.jones@linaro.org; linux-gpio@vger.kernel.org; linux-
> kernel@vger.kernel.org; Linus Walleij <linus.walleij@linaro.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 v3 2/3] x86/platform/p2sb: New Primary to Sideband
> bridge support driver for Intel SOC's
> 
> On Tue, Jun 21, 2016 at 05:03:20AM +0000, Tan, Jui Nee wrote:
> > > Maybe, or make it modular and use MODULE_DEVICE_TABLE(x86cpu, ...)
> > > to match the corresponding CPUs.
> >
> > We need CONFIG_X86_INTEL_NON_ACPI Kconfig option to select
> CONFIG_PINCTRL.
> > This is to solve kbuidbot complaint about kernel configuration, i.e.
> > CONFIG_PINCTRL=n. Appreciate if you could advise something on this.
> 
> Good point.
> 
> Then I guess you might add similar Kconfig option but can you call it
> something else than CONFIG_X86_INTEL_NON_ACPI. Perhaps something
> that relates to the actual product so distro people can then decide whether
> they want to support it or not. Along the lines of CONFIG_X86_INTEL_CE and
> so on.
Thanks for your suggestion. CONFIG_X86_INTEL_NON_ACPI will be renamed
to CONFIG_X86_INTEL_APL in next patch-set.

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

[Tan Jui Nee]

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

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

diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
index eea61e3..54e595c 100644
--- a/drivers/mfd/Kconfig
+++ b/drivers/mfd/Kconfig
@@ -359,8 +359,9 @@ config MFD_INTEL_QUARK_I2C_GPIO
 
 config LPC_ICH
 	tristate "Intel ICH LPC"
-	depends on PCI
+	depends on X86 && PCI
 	select MFD_CORE
+	select P2SB if X86_INTEL_NON_ACPI
 	help
 	  The LPC bridge function of the Intel ICH provides support for
 	  many functional units. This driver provides needed support for
diff --git a/drivers/mfd/lpc_ich.c b/drivers/mfd/lpc_ich.c
index bd3aa45..54076ee 100644
--- a/drivers/mfd/lpc_ich.c
+++ b/drivers/mfd/lpc_ich.c
@@ -68,6 +68,10 @@
 #include <linux/mfd/core.h>
 #include <linux/mfd/lpc_ich.h>
 #include <linux/platform_data/itco_wdt.h>
+#include <linux/pinctrl/pinctrl.h>
+#include <linux/types.h>
+
+#include <asm/p2sb.h>
 
 #define ACPIBASE		0x40
 #define ACPIBASE_GPE_OFF	0x28
@@ -94,6 +98,21 @@
 #define wdt_mem_res(i) wdt_res(ICH_RES_MEM_OFF, i)
 #define wdt_res(b, i) (&wdt_ich_res[(b) + (i)])
 
+/* Offset data for Apollo Lake GPIO communities */
+#define APL_GPIO_SOUTHWEST_OFFSET	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
+
 struct lpc_ich_priv {
 	int chipset;
 
@@ -133,6 +152,76 @@ static struct resource gpio_ich_res[] = {
 	},
 };
 
+#ifdef CONFIG_X86_INTEL_NON_ACPI
+static struct resource apl_gpio_io_res[4][2] = {
+	{
+		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"),
+		{
+		},
+	},
+};
+
+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[1],
+		.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[1],
+		.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[1],
+		.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[1],
+		.pdata_size = sizeof(apl_pinctrl_pdata),
+		.platform_data = &apl_pinctrl_pdata,
+		.ignore_resource_conflicts = true,
+	},
+};
+#endif /* CONFIG_X86_INTEL_NON_ACPI */
+
 static struct mfd_cell lpc_ich_wdt_cell = {
 	.name = "iTCO_wdt",
 	.num_resources = ARRAY_SIZE(wdt_ich_res),
@@ -216,6 +305,7 @@ enum lpc_chipsets {
 	LPC_BRASWELL,	/* Braswell SoC */
 	LPC_LEWISBURG,	/* Lewisburg */
 	LPC_9S,		/* 9 Series */
+	LPC_APL,	/* Apollo Lake SoC */
 };
 
 static struct lpc_ich_info lpc_chipset_info[] = {
@@ -531,6 +621,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 +773,7 @@ static const struct pci_device_id lpc_ich_ids[] = {
 	{ PCI_VDEVICE(INTEL, 0x3b14), LPC_3420},
 	{ PCI_VDEVICE(INTEL, 0x3b16), LPC_3450},
 	{ PCI_VDEVICE(INTEL, 0x5031), LPC_EP80579},
+	{ PCI_VDEVICE(INTEL, 0x5ae8), LPC_APL},
 	{ PCI_VDEVICE(INTEL, 0x8c40), LPC_LPT},
 	{ PCI_VDEVICE(INTEL, 0x8c41), LPC_LPT},
 	{ PCI_VDEVICE(INTEL, 0x8c42), LPC_LPT},
@@ -1050,6 +1145,61 @@ wdt_done:
 	return ret;
 }
 
+#ifdef CONFIG_X86_INTEL_NON_ACPI
+static int lpc_ich_misc(struct pci_dev *dev, enum lpc_chipsets chipset)
+{
+	unsigned int apl_p2sb = PCI_DEVFN(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); 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;
+
+		/* Fill IRQ resource */
+		res->start = APL_GPIO_IRQ;
+		res->end = res->start;
+		res->flags = IORESOURCE_IRQ;
+
+		apl_pinctrl_pdata.name = kasprintf(GFP_KERNEL, "%u",
+			i + 1);
+	}
+	if (apl_pinctrl_pdata.name)
+		ret = mfd_add_devices(&dev->dev, apl_gpio_devices->id,
+			apl_gpio_devices, ARRAY_SIZE(apl_gpio_devices),
+				NULL, 0, NULL);
+	else
+		ret = -ENOMEM;
+
+warn_continue:
+	if (ret)
+		dev_warn(&dev->dev,
+			"Failed to add Apollo Lake GPIO %s: %d\n",
+				apl_pinctrl_pdata.name, ret);
+
+	kfree(apl_pinctrl_pdata.name);
+	return 0;
+}
+#else
+static inline int lpc_ich_misc(struct pci_dev *dev, enum lpc_chipsets chipset)
+{
+	return -ENODEV;
+}
+#endif /* CONFIG_X86_INTEL_NON_ACPI */
+
 static int lpc_ich_probe(struct pci_dev *dev,
 				const struct pci_device_id *id)
 {
@@ -1093,6 +1243,9 @@ static int lpc_ich_probe(struct pci_dev *dev,
 			cell_added = true;
 	}
 
+	if (!lpc_ich_misc(dev, priv->chipset))
+		cell_added = true;
+
 	/*
 	 * We only care if at least one or none of the cells registered
 	 * successfully.
-- 
1.9.1

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

[Lee Jones]

On Tue, 07 Jun 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>
> ---

You really need to supply a changelog here when you send subsequent
patch revisions.

>  drivers/mfd/Kconfig   |   3 +-
>  drivers/mfd/lpc_ich.c | 153 ++++++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 155 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> index eea61e3..54e595c 100644
> --- a/drivers/mfd/Kconfig
> +++ b/drivers/mfd/Kconfig
> @@ -359,8 +359,9 @@ config MFD_INTEL_QUARK_I2C_GPIO
>  
>  config LPC_ICH
>  	tristate "Intel ICH LPC"
> -	depends on PCI
> +	depends on X86 && PCI
>  	select MFD_CORE
> +	select P2SB if X86_INTEL_NON_ACPI
>  	help
>  	  The LPC bridge function of the Intel ICH provides support for
>  	  many functional units. This driver provides needed support for
> diff --git a/drivers/mfd/lpc_ich.c b/drivers/mfd/lpc_ich.c
> index bd3aa45..54076ee 100644
> --- a/drivers/mfd/lpc_ich.c
> +++ b/drivers/mfd/lpc_ich.c
> @@ -68,6 +68,10 @@
>  #include <linux/mfd/core.h>
>  #include <linux/mfd/lpc_ich.h>
>  #include <linux/platform_data/itco_wdt.h>
> +#include <linux/pinctrl/pinctrl.h>
> +#include <linux/types.h>
> +
> +#include <asm/p2sb.h>
>  
>  #define ACPIBASE		0x40
>  #define ACPIBASE_GPE_OFF	0x28
> @@ -94,6 +98,21 @@
>  #define wdt_mem_res(i) wdt_res(ICH_RES_MEM_OFF, i)
>  #define wdt_res(b, i) (&wdt_ich_res[(b) + (i)])
>  
> +/* Offset data for Apollo Lake GPIO communities */
> +#define APL_GPIO_SOUTHWEST_OFFSET	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
> +
>  struct lpc_ich_priv {
>  	int chipset;
>  
> @@ -133,6 +152,76 @@ static struct resource gpio_ich_res[] = {
>  	},
>  };
>  
> +#ifdef CONFIG_X86_INTEL_NON_ACPI

I already said you should remove theses.

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

I still don't get why you need a 2D array?

> +	{
> +		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"),
> +		{
> +		},
> +	},
> +};
> +
> +static struct pinctrl_pin_desc apl_pinctrl_pdata;

Is this forward declaration avoidable at all?

> +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[1],
> +		.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[1],
> +		.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[1],
> +		.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[1],
> +		.pdata_size = sizeof(apl_pinctrl_pdata),
> +		.platform_data = &apl_pinctrl_pdata,
> +		.ignore_resource_conflicts = true,
> +	},
> +};
> +#endif /* CONFIG_X86_INTEL_NON_ACPI */
> +
>  static struct mfd_cell lpc_ich_wdt_cell = {
>  	.name = "iTCO_wdt",
>  	.num_resources = ARRAY_SIZE(wdt_ich_res),
> @@ -216,6 +305,7 @@ enum lpc_chipsets {
>  	LPC_BRASWELL,	/* Braswell SoC */
>  	LPC_LEWISBURG,	/* Lewisburg */
>  	LPC_9S,		/* 9 Series */
> +	LPC_APL,	/* Apollo Lake SoC */
>  };
>  
>  static struct lpc_ich_info lpc_chipset_info[] = {
> @@ -531,6 +621,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 +773,7 @@ static const struct pci_device_id lpc_ich_ids[] = {
>  	{ PCI_VDEVICE(INTEL, 0x3b14), LPC_3420},
>  	{ PCI_VDEVICE(INTEL, 0x3b16), LPC_3450},
>  	{ PCI_VDEVICE(INTEL, 0x5031), LPC_EP80579},
> +	{ PCI_VDEVICE(INTEL, 0x5ae8), LPC_APL},
>  	{ PCI_VDEVICE(INTEL, 0x8c40), LPC_LPT},
>  	{ PCI_VDEVICE(INTEL, 0x8c41), LPC_LPT},
>  	{ PCI_VDEVICE(INTEL, 0x8c42), LPC_LPT},
> @@ -1050,6 +1145,61 @@ wdt_done:
>  	return ret;
>  }
>  
> +#ifdef CONFIG_X86_INTEL_NON_ACPI
> +static int lpc_ich_misc(struct pci_dev *dev, enum lpc_chipsets chipset)
> +{
> +	unsigned int apl_p2sb = PCI_DEVFN(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); 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;
> +
> +		/* Fill IRQ resource */
> +		res->start = APL_GPIO_IRQ;
> +		res->end = res->start;
> +		res->flags = IORESOURCE_IRQ;
> +
> +		apl_pinctrl_pdata.name = kasprintf(GFP_KERNEL, "%u",
> +			i + 1);
> +	}
> +	if (apl_pinctrl_pdata.name)
> +		ret = mfd_add_devices(&dev->dev, apl_gpio_devices->id,
> +			apl_gpio_devices, ARRAY_SIZE(apl_gpio_devices),
> +				NULL, 0, NULL);
> +	else
> +		ret = -ENOMEM;
> +
> +warn_continue:
> +	if (ret)
> +		dev_warn(&dev->dev,
> +			"Failed to add Apollo Lake GPIO %s: %d\n",
> +				apl_pinctrl_pdata.name, ret);
> +
> +	kfree(apl_pinctrl_pdata.name);
> +	return 0;
> +}
> +#else
> +static inline int lpc_ich_misc(struct pci_dev *dev, enum lpc_chipsets chipset)
> +{
> +	return -ENODEV;
> +}
> +#endif /* CONFIG_X86_INTEL_NON_ACPI */
> +
>  static int lpc_ich_probe(struct pci_dev *dev,
>  				const struct pci_device_id *id)
>  {
> @@ -1093,6 +1243,9 @@ static int lpc_ich_probe(struct pci_dev *dev,
>  			cell_added = true;
>  	}

Use:

if defined(CONFIG_X86_INTEL_NON_ACPI)

... here and the compiler will do the rest.

Although, where is this defined?  I don't see it anywhere.

> +	if (!lpc_ich_misc(dev, priv->chipset))
> +		cell_added = true;
> +
>  	/*
>  	 * We only care if at least one or none of the cells registered
>  	 * successfully.

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

RE: [PATCH v3 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: Thursday, June 9, 2016 11:56 PM
> To: Tan, Jui Nee <jui.nee.tan@intel.com>
> Cc: mika.westerberg@linux.intel.com; heikki.krogerus@linux.intel.com;
> andriy.shevchenko@linux.intel.com; tglx@linutronix.de;
> mingo@redhat.com; hpa@zytor.com; x86@kernel.org; ptyser@xes-inc.com;
> linux-gpio@vger.kernel.org; linux-kernel@vger.kernel.org; Yong, Jonathan
> <jonathan.yong@intel.com>; Yu, Ong Hock <ong.hock.yu@intel.com>; Voon,
> Weifeng <weifeng.voon@intel.com>; Wan Mohamad, Wan Ahmad Zainie
> <wan.ahmad.zainie.wan.mohamad@intel.com>
> Subject: Re: [PATCH v3 3/3] mfd: lpc_ich: Add support for Intel Apollo Lake
> GPIO pinctrl in non-ACPI system
> 
> On Tue, 07 Jun 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>
> > ---
> 
> You really need to supply a changelog here when you send subsequent patch
> revisions.
> 
The changelog was already added in patchset cover letter  
(https://lkml.org/lkml/2016/6/7/91). 
In next subsequent patch revisions, I will add the changelog on every
related patch instead of just cover letter.
> >  drivers/mfd/Kconfig   |   3 +-
> >  drivers/mfd/lpc_ich.c | 153
> > ++++++++++++++++++++++++++++++++++++++++++++++++++
> >  2 files changed, 155 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig index
> > eea61e3..54e595c 100644
> > --- a/drivers/mfd/Kconfig
> > +++ b/drivers/mfd/Kconfig
> > @@ -359,8 +359,9 @@ config MFD_INTEL_QUARK_I2C_GPIO
> >
> >  config LPC_ICH
> >  	tristate "Intel ICH LPC"
> > -	depends on PCI
> > +	depends on X86 && PCI
> >  	select MFD_CORE
> > +	select P2SB if X86_INTEL_NON_ACPI
> >  	help
> >  	  The LPC bridge function of the Intel ICH provides support for
> >  	  many functional units. This driver provides needed support for
> > diff --git a/drivers/mfd/lpc_ich.c b/drivers/mfd/lpc_ich.c index
> > bd3aa45..54076ee 100644
> > --- a/drivers/mfd/lpc_ich.c
> > +++ b/drivers/mfd/lpc_ich.c
> > @@ -68,6 +68,10 @@
> >  #include <linux/mfd/core.h>
> >  #include <linux/mfd/lpc_ich.h>
> >  #include <linux/platform_data/itco_wdt.h>
> > +#include <linux/pinctrl/pinctrl.h>
> > +#include <linux/types.h>
> > +
> > +#include <asm/p2sb.h>
> >
> >  #define ACPIBASE		0x40
> >  #define ACPIBASE_GPE_OFF	0x28
> > @@ -94,6 +98,21 @@
> >  #define wdt_mem_res(i) wdt_res(ICH_RES_MEM_OFF, i)  #define
> > wdt_res(b, i) (&wdt_ich_res[(b) + (i)])
> >
> > +/* Offset data for Apollo Lake GPIO communities */
> > +#define APL_GPIO_SOUTHWEST_OFFSET	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
> > +
> >  struct lpc_ich_priv {
> >  	int chipset;
> >
> > @@ -133,6 +152,76 @@ static struct resource gpio_ich_res[] = {
> >  	},
> >  };
> >
> > +#ifdef CONFIG_X86_INTEL_NON_ACPI
> 
> I already said you should remove theses.
> 
CONFIG_X86_INTEL_NON_ACPI will be removed in next patch-set.
> > +static struct resource apl_gpio_io_res[4][2] = {
> 
> I still don't get why you need a 2D array?
> 
Every GPIO community has its own IRQ resource. Since all the GPIO communities
(total 4) have the same IRQ resource, i.e. 14, I have simplified the array as
below:
	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),
	};
	...
I have tested this on Intel Apollo Lake platform and it works fine.
Please advise if you have any concerns.
> > +	{
> > +		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"),
> > +		{
> > +		},
> > +	},
> > +};
> > +
> > +static struct pinctrl_pin_desc apl_pinctrl_pdata;
> 
> Is this forward declaration avoidable at all?
> 
Instead of using pinctrl_pin_desc struct which is located at
include/linux/pinctrl/pinctrl.h file, I have tried to create a new struct to
replace the forward declaration, that is as follows:
	static struct pinctrl_port {
		char *name;
	};
	...
I have tested this on Intel Apollo Lake platform and it works. 
However, Andy commented that the new struct will make things worse. 
Andy, probably you could share your thoughts on this.
> > +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[1],
> > +		.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[1],
> > +		.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[1],
> > +		.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[1],
> > +		.pdata_size = sizeof(apl_pinctrl_pdata),
> > +		.platform_data = &apl_pinctrl_pdata,
> > +		.ignore_resource_conflicts = true,
> > +	},
> > +};
> > +#endif /* CONFIG_X86_INTEL_NON_ACPI */
> > +
> >  static struct mfd_cell lpc_ich_wdt_cell = {
> >  	.name = "iTCO_wdt",
> >  	.num_resources = ARRAY_SIZE(wdt_ich_res), @@ -216,6 +305,7 @@
> enum
> > lpc_chipsets {
> >  	LPC_BRASWELL,	/* Braswell SoC */
> >  	LPC_LEWISBURG,	/* Lewisburg */
> >  	LPC_9S,		/* 9 Series */
> > +	LPC_APL,	/* Apollo Lake SoC */
> >  };
> >
> >  static struct lpc_ich_info lpc_chipset_info[] = { @@ -531,6 +621,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 +773,7 @@ static const struct pci_device_id lpc_ich_ids[] = {
> >  	{ PCI_VDEVICE(INTEL, 0x3b14), LPC_3420},
> >  	{ PCI_VDEVICE(INTEL, 0x3b16), LPC_3450},
> >  	{ PCI_VDEVICE(INTEL, 0x5031), LPC_EP80579},
> > +	{ PCI_VDEVICE(INTEL, 0x5ae8), LPC_APL},
> >  	{ PCI_VDEVICE(INTEL, 0x8c40), LPC_LPT},
> >  	{ PCI_VDEVICE(INTEL, 0x8c41), LPC_LPT},
> >  	{ PCI_VDEVICE(INTEL, 0x8c42), LPC_LPT}, @@ -1050,6 +1145,61 @@
> > wdt_done:
> >  	return ret;
> >  }
> >
> > +#ifdef CONFIG_X86_INTEL_NON_ACPI
> > +static int lpc_ich_misc(struct pci_dev *dev, enum lpc_chipsets
> > +chipset) {
> > +	unsigned int apl_p2sb = PCI_DEVFN(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); 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;
> > +
> > +		/* Fill IRQ resource */
> > +		res->start = APL_GPIO_IRQ;
> > +		res->end = res->start;
> > +		res->flags = IORESOURCE_IRQ;
> > +
> > +		apl_pinctrl_pdata.name = kasprintf(GFP_KERNEL, "%u",
> > +			i + 1);
> > +	}
> > +	if (apl_pinctrl_pdata.name)
> > +		ret = mfd_add_devices(&dev->dev, apl_gpio_devices->id,
> > +			apl_gpio_devices, ARRAY_SIZE(apl_gpio_devices),
> > +				NULL, 0, NULL);
> > +	else
> > +		ret = -ENOMEM;
> > +
> > +warn_continue:
> > +	if (ret)
> > +		dev_warn(&dev->dev,
> > +			"Failed to add Apollo Lake GPIO %s: %d\n",
> > +				apl_pinctrl_pdata.name, ret);
> > +
> > +	kfree(apl_pinctrl_pdata.name);
> > +	return 0;
> > +}
> > +#else
> > +static inline int lpc_ich_misc(struct pci_dev *dev, enum lpc_chipsets
> > +chipset) {
> > +	return -ENODEV;
> > +}
> > +#endif /* CONFIG_X86_INTEL_NON_ACPI */
> > +
> >  static int lpc_ich_probe(struct pci_dev *dev,
> >  				const struct pci_device_id *id)
> >  {
> > @@ -1093,6 +1243,9 @@ static int lpc_ich_probe(struct pci_dev *dev,
> >  			cell_added = true;
> >  	}
> 
> Use:
> 
> if defined(CONFIG_X86_INTEL_NON_ACPI)
> 
> ... here and the compiler will do the rest.
> 
> Although, where is this defined?  I don't see it anywhere.
> 
Thanks for your suggestion. The changes will be applied in next patch-set.
CONFIG_X86_INTEL_NON_ACPI is defined in /arch/x86/Kconfig. 
I will move over the CONFIG_X86_INTEL_NON_ACPI define from patch
[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. 
Please advise if you have any concerns.
> > +	if (!lpc_ich_misc(dev, priv->chipset))
> > +		cell_added = true;
> > +
> >  	/*
> >  	 * We only care if at least one or none of the cells registered
> >  	 * successfully.
> 
> --
> Lee Jones
> Linaro STMicroelectronics Landing Team Lead Linaro.org │ Open source
> software for ARM SoCs Follow Linaro: Facebook | Twitter | Blog