diff mbox series

[v4,5/8] mfd: lpc_ich: Add support for pinctrl in non-ACPI system

Message ID 20220131151346.45792-6-andriy.shevchenko@linux.intel.com
State New
Headers show
Series platform/x86: introduce p2sb_bar() helper | expand

Commit Message

Andy Shevchenko Jan. 31, 2022, 3:13 p.m. UTC 
From: Tan Jui Nee <jui.nee.tan@intel.com>

Add support for non-ACPI systems, such as system that uses
Advanced Boot Loader (ABL) whereby a platform device has to be created
in order to bind with pin control and GPIO.

At the moment, Intel Apollo Lake In-Vehicle Infotainment (IVI) system
requires a driver to hide and unhide P2SB to lookup P2SB BAR and pass
the PCI BAR address to GPIO.

Signed-off-by: Tan Jui Nee <jui.nee.tan@intel.com>
Co-developed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Acked-by: Hans de Goede <hdegoede@redhat.com>
Acked-by: Linus Walleij <linus.walleij@linaro.org>
---
 drivers/mfd/lpc_ich.c | 101 +++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 100 insertions(+), 1 deletion(-)

Comments

Lee Jones Feb. 15, 2022, 5:29 p.m. UTC | #1 
On Tue, 15 Feb 2022, Andy Shevchenko wrote:

> On Tue, Feb 15, 2022 at 04:54:00PM +0000, Lee Jones wrote:
> > On Mon, 31 Jan 2022, Andy Shevchenko wrote:
> 
> Thank you for the review, my answers below.
> 
> ...
> 
> > > +static struct resource apl_gpio_resources[APL_GPIO_NR_DEVICES][2] = {
> > > +	[APL_GPIO_NORTH] = {
> > > +		DEFINE_RES_MEM(APL_GPIO_NORTH_OFFSET, 0x1000),
> > 
> > Are these 0x1000's being over-written in lpc_ich_init_pinctrl()?
> > 
> > If so, why pre-initialise?
> 
> You mean to pre-initialize the offsets, but leave the length to be added
> in the function? It can be done, but it feels inconsistent, since we would
> have offsets and lengths in different places for the same thingy. That said,
> I prefer current way for the sake of consistency.

Don't you over-write this entry entirely?

  for (i = 0; i < ARRAY_SIZE(apl_gpio_devices); i++) {
        struct resource *mem = &apl_gpio_resources[i][0];

        /* Fill MEM resource */
        mem->start += base.start;
        mem->end += base.start;
        mem->flags = base.flags;
  }

Oh wait, you're just adding the base value to the offsets.

In which case that comment is also confusing!
Henning Schild March 7, 2022, 6:21 p.m. UTC | #2 
Can this patch not be proposed separately? Maybe i am wrong but it
seems unrelated to the p2sb story.

The whole p2sb base and size discovery is easy and switching the
simatic drivers is also. It is an interface change, where the old open
coding remains working.

But having to switch to GPIO in the same series is kind of weird. That
is a functional change which even might deserve its own cover letter. I
bet there are tons of out-of-tree modules which will stop working on
apl after that gets merged.

I still did not understand why apl is special and other boards do not
get their pinctrl brought up without ACPI/p2sb-visible.

I have patches floating around, but still would be happy if we could do
one thing at a time.

Or maybe it is strongly coupled and i do not understand why.

regards,
Henning

Am Mon, 31 Jan 2022 17:13:43 +0200
schrieb Andy Shevchenko <andriy.shevchenko@linux.intel.com>:

> From: Tan Jui Nee <jui.nee.tan@intel.com>
> 
> Add support for non-ACPI systems, such as system that uses
> Advanced Boot Loader (ABL) whereby a platform device has to be created
> in order to bind with pin control and GPIO.
> 
> At the moment, Intel Apollo Lake In-Vehicle Infotainment (IVI) system
> requires a driver to hide and unhide P2SB to lookup P2SB BAR and pass
> the PCI BAR address to GPIO.
> 
> Signed-off-by: Tan Jui Nee <jui.nee.tan@intel.com>
> Co-developed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Acked-by: Hans de Goede <hdegoede@redhat.com>
> Acked-by: Linus Walleij <linus.walleij@linaro.org>
> ---
>  drivers/mfd/lpc_ich.c | 101
> +++++++++++++++++++++++++++++++++++++++++- 1 file changed, 100
> insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/mfd/lpc_ich.c b/drivers/mfd/lpc_ich.c
> index 95dca5434917..e1bca5325ce7 100644
> --- a/drivers/mfd/lpc_ich.c
> +++ b/drivers/mfd/lpc_ich.c
> @@ -8,7 +8,8 @@
>   *  Configuration Registers.
>   *
>   *  This driver is derived from lpc_sch.
> -
> + *
> + *  Copyright (c) 2017, 2021-2022 Intel Corporation
>   *  Copyright (c) 2011 Extreme Engineering Solution, Inc.
>   *  Author: Aaron Sierra <asierra@xes-inc.com>
>   *
> @@ -42,6 +43,7 @@
>  #include <linux/errno.h>
>  #include <linux/acpi.h>
>  #include <linux/pci.h>
> +#include <linux/pinctrl/pinctrl.h>
>  #include <linux/mfd/core.h>
>  #include <linux/mfd/lpc_ich.h>
>  #include <linux/platform_data/itco_wdt.h>
> @@ -140,6 +142,70 @@ static struct mfd_cell lpc_ich_gpio_cell = {
>  	.ignore_resource_conflicts = true,
>  };
>  
> +#define APL_GPIO_NORTH		0
> +#define APL_GPIO_NORTHWEST	1
> +#define APL_GPIO_WEST		2
> +#define APL_GPIO_SOUTHWEST	3
> +#define APL_GPIO_NR_DEVICES	4
> +
> +/* Offset data for Apollo Lake GPIO controllers */
> +#define APL_GPIO_NORTH_OFFSET		0xc50000
> +#define APL_GPIO_NORTHWEST_OFFSET	0xc40000
> +#define APL_GPIO_WEST_OFFSET		0xc70000
> +#define APL_GPIO_SOUTHWEST_OFFSET	0xc00000
> +
> +#define APL_GPIO_IRQ			14
> +
> +static struct resource apl_gpio_resources[APL_GPIO_NR_DEVICES][2] = {
> +	[APL_GPIO_NORTH] = {
> +		DEFINE_RES_MEM(APL_GPIO_NORTH_OFFSET, 0x1000),
> +		DEFINE_RES_IRQ(APL_GPIO_IRQ),
> +	},
> +	[APL_GPIO_NORTHWEST] = {
> +		DEFINE_RES_MEM(APL_GPIO_NORTHWEST_OFFSET, 0x1000),
> +		DEFINE_RES_IRQ(APL_GPIO_IRQ),
> +	},
> +	[APL_GPIO_WEST] = {
> +		DEFINE_RES_MEM(APL_GPIO_WEST_OFFSET, 0x1000),
> +		DEFINE_RES_IRQ(APL_GPIO_IRQ),
> +	},
> +	[APL_GPIO_SOUTHWEST] = {
> +		DEFINE_RES_MEM(APL_GPIO_SOUTHWEST_OFFSET, 0x1000),
> +		DEFINE_RES_IRQ(APL_GPIO_IRQ),
> +	},
> +};
> +
> +/* The order must be in sync with apl_pinctrl_soc_data */
> +static const struct mfd_cell apl_gpio_devices[APL_GPIO_NR_DEVICES] =
> {
> +	[APL_GPIO_NORTH] = {
> +		.name = "apollolake-pinctrl",
> +		.id = APL_GPIO_NORTH,
> +		.num_resources =
> ARRAY_SIZE(apl_gpio_resources[APL_GPIO_NORTH]),
> +		.resources = apl_gpio_resources[APL_GPIO_NORTH],
> +		.ignore_resource_conflicts = true,
> +	},
> +	[APL_GPIO_NORTHWEST] = {
> +		.name = "apollolake-pinctrl",
> +		.id = APL_GPIO_NORTHWEST,
> +		.num_resources =
> ARRAY_SIZE(apl_gpio_resources[APL_GPIO_NORTHWEST]),
> +		.resources = apl_gpio_resources[APL_GPIO_NORTHWEST],
> +		.ignore_resource_conflicts = true,
> +	},
> +	[APL_GPIO_WEST] = {
> +		.name = "apollolake-pinctrl",
> +		.id = APL_GPIO_WEST,
> +		.num_resources =
> ARRAY_SIZE(apl_gpio_resources[APL_GPIO_WEST]),
> +		.resources = apl_gpio_resources[APL_GPIO_WEST],
> +		.ignore_resource_conflicts = true,
> +	},
> +	[APL_GPIO_SOUTHWEST] = {
> +		.name = "apollolake-pinctrl",
> +		.id = APL_GPIO_SOUTHWEST,
> +		.num_resources =
> ARRAY_SIZE(apl_gpio_resources[APL_GPIO_SOUTHWEST]),
> +		.resources = apl_gpio_resources[APL_GPIO_SOUTHWEST],
> +		.ignore_resource_conflicts = true,
> +	},
> +};
>  
>  static struct mfd_cell lpc_ich_spi_cell = {
>  	.name = "intel-spi",
> @@ -1083,6 +1149,33 @@ static int lpc_ich_init_wdt(struct pci_dev
> *dev) return ret;
>  }
>  
> +static int lpc_ich_init_pinctrl(struct pci_dev *dev)
> +{
> +	struct resource base;
> +	unsigned int i;
> +	int ret;
> +
> +	/* Check, if GPIO has been exported as an ACPI device */
> +	if (acpi_dev_present("INT3452", NULL, -1))
> +		return -EEXIST;
> +
> +	ret = p2sb_bar(dev->bus, 0, &base);
> +	if (ret)
> +		return ret;
> +
> +	for (i = 0; i < ARRAY_SIZE(apl_gpio_devices); i++) {
> +		struct resource *mem = &apl_gpio_resources[i][0];
> +
> +		/* Fill MEM resource */
> +		mem->start += base.start;
> +		mem->end += base.start;
> +		mem->flags = base.flags;
> +	}
> +
> +	return mfd_add_devices(&dev->dev, 0, apl_gpio_devices,
> +			       ARRAY_SIZE(apl_gpio_devices), NULL,
> 0, NULL); +}
> +
>  static void lpc_ich_test_spi_write(struct pci_dev *dev, unsigned int
> devfn, struct intel_spi_boardinfo *info)
>  {
> @@ -1199,6 +1292,12 @@ static int lpc_ich_probe(struct pci_dev *dev,
>  			cell_added = true;
>  	}
>  
> +	if (priv->chipset == LPC_APL) {
> +		ret = lpc_ich_init_pinctrl(dev);
> +		if (!ret)
> +			cell_added = true;
> +	}
> +
>  	if (lpc_chipset_info[priv->chipset].spi_type) {
>  		ret = lpc_ich_init_spi(dev);
>  		if (!ret)
Andy Shevchenko March 7, 2022, 7:03 p.m. UTC | #3 
On Mon, Mar 7, 2022 at 8:21 PM Henning Schild
<henning.schild@siemens.com> wrote:

Please, do not top-post.

> Can this patch not be proposed separately? Maybe i am wrong but it
> seems unrelated to the p2sb story.

The entire story happens to begin from this very change. The author
(you may see that's not me) proposed the change a long time ago and
AFAIU this is the requirement to have it upstreamed.

> The whole p2sb base and size discovery is easy and switching the
> simatic drivers is also. It is an interface change, where the old open
> coding remains working.
>
> But having to switch to GPIO in the same series is kind of weird. That
> is a functional change which even might deserve its own cover letter. I
> bet there are tons of out-of-tree modules which will stop working on
> apl after that gets merged.

Upstream rarely, if at all, cares about 3rd party modules. From the
upstream point of view the thing (whatever the 3rd party module
supports) wasn't working ("no driver" in upstream) and won't work
(still "no driver" in upstream) after the change, so there may not be
any regression.

> I still did not understand why apl is special and other boards do not
> get their pinctrl brought up without ACPI/p2sb-visible.

The platform is being heavily used by one of our departments in such
configuration with firmwares that may not be fully compatible with
UEFI.They want to support that along with the case when BIOS has no
GPIO device being provided.

> I have patches floating around, but still would be happy if we could do
> one thing at a time.

Either way any new changes must use a pin control driver and the
previous work was accepted only on this basis.

> Or maybe it is strongly coupled and I do not understand why.

That's the initial requirement by our peer departament.
Andy Shevchenko May 4, 2022, 12:52 p.m. UTC | #4 
On Tue, Feb 15, 2022 at 05:29:51PM +0000, Lee Jones wrote:
> On Tue, 15 Feb 2022, Andy Shevchenko wrote:
> > On Tue, Feb 15, 2022 at 04:54:00PM +0000, Lee Jones wrote:
> > > On Mon, 31 Jan 2022, Andy Shevchenko wrote:

> > Thank you for the review, my answers below.

...

> > > > +static struct resource apl_gpio_resources[APL_GPIO_NR_DEVICES][2] = {
> > > > +	[APL_GPIO_NORTH] = {
> > > > +		DEFINE_RES_MEM(APL_GPIO_NORTH_OFFSET, 0x1000),
> > > 
> > > Are these 0x1000's being over-written in lpc_ich_init_pinctrl()?
> > > 
> > > If so, why pre-initialise?
> > 
> > You mean to pre-initialize the offsets, but leave the length to be added
> > in the function? It can be done, but it feels inconsistent, since we would
> > have offsets and lengths in different places for the same thingy. That said,
> > I prefer current way for the sake of consistency.
> 
> Don't you over-write this entry entirely?
> 
>   for (i = 0; i < ARRAY_SIZE(apl_gpio_devices); i++) {
>         struct resource *mem = &apl_gpio_resources[i][0];
> 
>         /* Fill MEM resource */
>         mem->start += base.start;
>         mem->end += base.start;
>         mem->flags = base.flags;
>   }
> 
> Oh wait, you're just adding the base value to the offsets.
> 
> In which case that comment is also confusing!

I have realised that in current form it has a bug (*), so I re-do a bit the way
that comment stays and the actual actions will be to *fill* the resource.

*) unbinding and binding back will bring us to the completely wrong resources.
diff mbox series

Patch

diff --git a/drivers/mfd/lpc_ich.c b/drivers/mfd/lpc_ich.c
index 95dca5434917..e1bca5325ce7 100644
--- a/drivers/mfd/lpc_ich.c
+++ b/drivers/mfd/lpc_ich.c
@@ -8,7 +8,8 @@ 
  *  Configuration Registers.
  *
  *  This driver is derived from lpc_sch.
-
+ *
+ *  Copyright (c) 2017, 2021-2022 Intel Corporation
  *  Copyright (c) 2011 Extreme Engineering Solution, Inc.
  *  Author: Aaron Sierra <asierra@xes-inc.com>
  *
@@ -42,6 +43,7 @@ 
 #include <linux/errno.h>
 #include <linux/acpi.h>
 #include <linux/pci.h>
+#include <linux/pinctrl/pinctrl.h>
 #include <linux/mfd/core.h>
 #include <linux/mfd/lpc_ich.h>
 #include <linux/platform_data/itco_wdt.h>
@@ -140,6 +142,70 @@  static struct mfd_cell lpc_ich_gpio_cell = {
 	.ignore_resource_conflicts = true,
 };
 
+#define APL_GPIO_NORTH		0
+#define APL_GPIO_NORTHWEST	1
+#define APL_GPIO_WEST		2
+#define APL_GPIO_SOUTHWEST	3
+#define APL_GPIO_NR_DEVICES	4
+
+/* Offset data for Apollo Lake GPIO controllers */
+#define APL_GPIO_NORTH_OFFSET		0xc50000
+#define APL_GPIO_NORTHWEST_OFFSET	0xc40000
+#define APL_GPIO_WEST_OFFSET		0xc70000
+#define APL_GPIO_SOUTHWEST_OFFSET	0xc00000
+
+#define APL_GPIO_IRQ			14
+
+static struct resource apl_gpio_resources[APL_GPIO_NR_DEVICES][2] = {
+	[APL_GPIO_NORTH] = {
+		DEFINE_RES_MEM(APL_GPIO_NORTH_OFFSET, 0x1000),
+		DEFINE_RES_IRQ(APL_GPIO_IRQ),
+	},
+	[APL_GPIO_NORTHWEST] = {
+		DEFINE_RES_MEM(APL_GPIO_NORTHWEST_OFFSET, 0x1000),
+		DEFINE_RES_IRQ(APL_GPIO_IRQ),
+	},
+	[APL_GPIO_WEST] = {
+		DEFINE_RES_MEM(APL_GPIO_WEST_OFFSET, 0x1000),
+		DEFINE_RES_IRQ(APL_GPIO_IRQ),
+	},
+	[APL_GPIO_SOUTHWEST] = {
+		DEFINE_RES_MEM(APL_GPIO_SOUTHWEST_OFFSET, 0x1000),
+		DEFINE_RES_IRQ(APL_GPIO_IRQ),
+	},
+};
+
+/* The order must be in sync with apl_pinctrl_soc_data */
+static const struct mfd_cell apl_gpio_devices[APL_GPIO_NR_DEVICES] = {
+	[APL_GPIO_NORTH] = {
+		.name = "apollolake-pinctrl",
+		.id = APL_GPIO_NORTH,
+		.num_resources = ARRAY_SIZE(apl_gpio_resources[APL_GPIO_NORTH]),
+		.resources = apl_gpio_resources[APL_GPIO_NORTH],
+		.ignore_resource_conflicts = true,
+	},
+	[APL_GPIO_NORTHWEST] = {
+		.name = "apollolake-pinctrl",
+		.id = APL_GPIO_NORTHWEST,
+		.num_resources = ARRAY_SIZE(apl_gpio_resources[APL_GPIO_NORTHWEST]),
+		.resources = apl_gpio_resources[APL_GPIO_NORTHWEST],
+		.ignore_resource_conflicts = true,
+	},
+	[APL_GPIO_WEST] = {
+		.name = "apollolake-pinctrl",
+		.id = APL_GPIO_WEST,
+		.num_resources = ARRAY_SIZE(apl_gpio_resources[APL_GPIO_WEST]),
+		.resources = apl_gpio_resources[APL_GPIO_WEST],
+		.ignore_resource_conflicts = true,
+	},
+	[APL_GPIO_SOUTHWEST] = {
+		.name = "apollolake-pinctrl",
+		.id = APL_GPIO_SOUTHWEST,
+		.num_resources = ARRAY_SIZE(apl_gpio_resources[APL_GPIO_SOUTHWEST]),
+		.resources = apl_gpio_resources[APL_GPIO_SOUTHWEST],
+		.ignore_resource_conflicts = true,
+	},
+};
 
 static struct mfd_cell lpc_ich_spi_cell = {
 	.name = "intel-spi",
@@ -1083,6 +1149,33 @@  static int lpc_ich_init_wdt(struct pci_dev *dev)
 	return ret;
 }
 
+static int lpc_ich_init_pinctrl(struct pci_dev *dev)
+{
+	struct resource base;
+	unsigned int i;
+	int ret;
+
+	/* Check, if GPIO has been exported as an ACPI device */
+	if (acpi_dev_present("INT3452", NULL, -1))
+		return -EEXIST;
+
+	ret = p2sb_bar(dev->bus, 0, &base);
+	if (ret)
+		return ret;
+
+	for (i = 0; i < ARRAY_SIZE(apl_gpio_devices); i++) {
+		struct resource *mem = &apl_gpio_resources[i][0];
+
+		/* Fill MEM resource */
+		mem->start += base.start;
+		mem->end += base.start;
+		mem->flags = base.flags;
+	}
+
+	return mfd_add_devices(&dev->dev, 0, apl_gpio_devices,
+			       ARRAY_SIZE(apl_gpio_devices), NULL, 0, NULL);
+}
+
 static void lpc_ich_test_spi_write(struct pci_dev *dev, unsigned int devfn,
 				   struct intel_spi_boardinfo *info)
 {
@@ -1199,6 +1292,12 @@  static int lpc_ich_probe(struct pci_dev *dev,
 			cell_added = true;
 	}
 
+	if (priv->chipset == LPC_APL) {
+		ret = lpc_ich_init_pinctrl(dev);
+		if (!ret)
+			cell_added = true;
+	}
+
 	if (lpc_chipset_info[priv->chipset].spi_type) {
 		ret = lpc_ich_init_spi(dev);
 		if (!ret)