[v1,6/7] mfd: lpc_ich: Add support for pinctrl in non-ACPI system

Message ID 20210308122020.57071-7-andriy.shevchenko@linux.intel.com
State New
Headers show
Series
  • PCI: introduce p2sb helper
Related show

Commit Message

Andy Shevchenko March 8, 2021, 12:20 p.m.
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>
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/mfd/lpc_ich.c | 100 +++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 99 insertions(+), 1 deletion(-)

Comments

Lee Jones March 10, 2021, 10:27 a.m. | #1
On Mon, 08 Mar 2021, Andy Shevchenko wrote:

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

> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

> ---

>  drivers/mfd/lpc_ich.c | 100 +++++++++++++++++++++++++++++++++++++++++-

>  1 file changed, 99 insertions(+), 1 deletion(-)

> 

> diff --git a/drivers/mfd/lpc_ich.c b/drivers/mfd/lpc_ich.c

> index 8e9bd6813287..959247b6987a 100644

> --- a/drivers/mfd/lpc_ich.c

> +++ b/drivers/mfd/lpc_ich.c

> @@ -8,7 +8,8 @@

>   *  Configuration Registers.

>   *

>   *  This driver is derived from lpc_sch.

> -

> + *

> + *  Copyright (C) 2017, 2021 Intel Corporation


Big C or little c?  Please be consistent.

>   *  Copyright (c) 2011 Extreme Engineering Solution, Inc.

>   *  Author: Aaron Sierra <asierra@xes-inc.com>

>   *

> @@ -43,6 +44,7 @@

>  #include <linux/acpi.h>

>  #include <linux/pci.h>

>  #include <linux/pci-p2sb.h>

> +#include <linux/pinctrl/pinctrl.h>

>  #include <linux/mfd/core.h>

>  #include <linux/mfd/lpc_ich.h>

>  #include <linux/platform_data/itco_wdt.h>

> @@ -140,6 +142,73 @@ static struct mfd_cell lpc_ich_gpio_cell = {

>  	.ignore_resource_conflicts = true,

>  };

>  

> +/* Offset data for Apollo Lake GPIO controllers */

> +#define APL_GPIO_SOUTHWEST_OFFSET	0xc00000

> +#define APL_GPIO_SOUTHWEST_SIZE		0x654

> +#define APL_GPIO_NORTHWEST_OFFSET	0xc40000

> +#define APL_GPIO_NORTHWEST_SIZE		0x764

> +#define APL_GPIO_NORTH_OFFSET		0xc50000

> +#define APL_GPIO_NORTH_SIZE		0x76c

> +#define APL_GPIO_WEST_OFFSET		0xc70000

> +#define APL_GPIO_WEST_SIZE		0x674

> +

> +#define APL_GPIO_NR_DEVICES		4

> +#define APL_GPIO_IRQ			14

> +

> +static struct resource apl_gpio_resources[APL_GPIO_NR_DEVICES][2] = {

> +	{

> +		DEFINE_RES_MEM(APL_GPIO_NORTH_OFFSET, APL_GPIO_NORTH_SIZE),

> +		DEFINE_RES_IRQ(APL_GPIO_IRQ),

> +	},

> +	{

> +		DEFINE_RES_MEM(APL_GPIO_NORTHWEST_OFFSET, APL_GPIO_NORTHWEST_SIZE),

> +		DEFINE_RES_IRQ(APL_GPIO_IRQ),

> +	},

> +	{

> +		DEFINE_RES_MEM(APL_GPIO_WEST_OFFSET, APL_GPIO_WEST_SIZE),

> +		DEFINE_RES_IRQ(APL_GPIO_IRQ),

> +	},

> +	{

> +		DEFINE_RES_MEM(APL_GPIO_SOUTHWEST_OFFSET, APL_GPIO_SOUTHWEST_SIZE),

> +		DEFINE_RES_IRQ(APL_GPIO_IRQ),

> +	},

> +};

> +

> +/* The order must be in sync with apl_pinctrl_soc_data */

> +static const struct mfd_cell apl_gpio_devices[APL_GPIO_NR_DEVICES] = {

> +	{

> +		/* North */

> +		.name = "apollolake-pinctrl",

> +		.id = 0,


Do these have to be hard-coded?

> +		.num_resources = ARRAY_SIZE(apl_gpio_resources[0]),

> +		.resources = apl_gpio_resources[0],


You can make this less fragile by defining the index and using:

  [DEFINE_X_Y_Z] = { /* resource */ }, /* etc */

... above.

> +		.ignore_resource_conflicts = true,

> +	},

> +	{

> +		/* NorthWest */

> +		.name = "apollolake-pinctrl",

> +		.id = 1,

> +		.num_resources = ARRAY_SIZE(apl_gpio_resources[1]),

> +		.resources = apl_gpio_resources[1],

> +		.ignore_resource_conflicts = true,

> +	},

> +	{

> +		/* West */

> +		.name = "apollolake-pinctrl",

> +		.id = 2,

> +		.num_resources = ARRAY_SIZE(apl_gpio_resources[2]),

> +		.resources = apl_gpio_resources[2],

> +		.ignore_resource_conflicts = true,

> +	},

> +	{

> +		/* SouthWest */

> +		.name = "apollolake-pinctrl",

> +		.id = 3,

> +		.num_resources = ARRAY_SIZE(apl_gpio_resources[3]),

> +		.resources = apl_gpio_resources[3],

> +		.ignore_resource_conflicts = true,

> +	},

> +};

>  

>  static struct mfd_cell lpc_ich_spi_cell = {

>  	.name = "intel-spi",

> @@ -1082,6 +1151,29 @@ static int lpc_ich_init_wdt(struct pci_dev *dev)

>  	return ret;

>  }

>  

> +static int lpc_ich_init_pinctrl(struct pci_dev *dev)

> +{

> +	struct resource base;

> +	unsigned int i;

> +	int ret;

> +

> +	ret = pci_p2sb_bar(dev, PCI_DEVFN(13, 0), &base);


What is 13 and 0?  Should these be defined?

> +	if (ret)

> +		return ret;

> +

> +	for (i = 0; i < ARRAY_SIZE(apl_gpio_devices); i++) {

> +		struct resource *mem = &apl_gpio_resources[i][0];

> +

> +		/* Fill MEM resource */

> +		mem->start += base.start;

> +		mem->end += base.start;

> +		mem->flags = base.flags;

> +	}


So you're converting PCI devices to platform devices.

I'm not sure how 'okay' that is.

Adding Greg to see if he has an opinion.

> +	return mfd_add_devices(&dev->dev, 0, apl_gpio_devices,


Please use the defines, rather than 0.

> +			       ARRAY_SIZE(apl_gpio_devices), NULL, 0, NULL);

> +}

> +

>  static void lpc_ich_test_spi_write(struct pci_dev *dev, unsigned int devfn,

>  				   struct intel_spi_boardinfo *info)

>  {

> @@ -1198,6 +1290,12 @@ static int lpc_ich_probe(struct pci_dev *dev,

>  			cell_added = true;

>  	}

>  

> +	if (priv->chipset == LPC_APL) {

> +		ret = lpc_ich_init_pinctrl(dev);

> +		if (!ret)

> +			cell_added = true;

> +	}

> +

>  	if (lpc_chipset_info[priv->chipset].spi_type) {

>  		ret = lpc_ich_init_spi(dev);

>  		if (!ret)


-- 
Lee Jones [李琼斯]
Senior Technical Lead - Developer Services
Linaro.org │ Open source software for Arm SoCs
Follow Linaro: Facebook | Twitter | Blog
Henning Schild April 12, 2021, 4:01 p.m. | #2
Am Mon, 8 Mar 2021 14:20:19 +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>

> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

> ---

>  drivers/mfd/lpc_ich.c | 100

> +++++++++++++++++++++++++++++++++++++++++- 1 file changed, 99

> insertions(+), 1 deletion(-)

> 

> diff --git a/drivers/mfd/lpc_ich.c b/drivers/mfd/lpc_ich.c

> index 8e9bd6813287..959247b6987a 100644

> --- a/drivers/mfd/lpc_ich.c

> +++ b/drivers/mfd/lpc_ich.c

> @@ -8,7 +8,8 @@

>   *  Configuration Registers.

>   *

>   *  This driver is derived from lpc_sch.

> -

> + *

> + *  Copyright (C) 2017, 2021 Intel Corporation

>   *  Copyright (c) 2011 Extreme Engineering Solution, Inc.

>   *  Author: Aaron Sierra <asierra@xes-inc.com>

>   *

> @@ -43,6 +44,7 @@

>  #include <linux/acpi.h>

>  #include <linux/pci.h>

>  #include <linux/pci-p2sb.h>

> +#include <linux/pinctrl/pinctrl.h>

>  #include <linux/mfd/core.h>

>  #include <linux/mfd/lpc_ich.h>

>  #include <linux/platform_data/itco_wdt.h>

> @@ -140,6 +142,73 @@ static struct mfd_cell lpc_ich_gpio_cell = {

>  	.ignore_resource_conflicts = true,

>  };

>  

> +/* Offset data for Apollo Lake GPIO controllers */

> +#define APL_GPIO_SOUTHWEST_OFFSET	0xc00000

> +#define APL_GPIO_SOUTHWEST_SIZE		0x654

> +#define APL_GPIO_NORTHWEST_OFFSET	0xc40000

> +#define APL_GPIO_NORTHWEST_SIZE		0x764

> +#define APL_GPIO_NORTH_OFFSET		0xc50000

> +#define APL_GPIO_NORTH_SIZE		0x76c


drivers/pinctrl/intel/pinctrl-broxton.c:653
BXT_COMMUNITY(0, 77),

> +#define APL_GPIO_WEST_OFFSET		0xc70000

> +#define APL_GPIO_WEST_SIZE		0x674


All these sizes correlate with 4 magic numbers from pinctrl-broxton.

SIZE - 0x500 (pad_base?) - 4 (no clue) / 8

It might be worth basing both numbers on a define and giving the magic
numbers some names.

But all this seems like duplication of pinctrl-broxton, maybe the
pinctrl driver should unhide the p2sb ...

regards,
Henning

> +

> +#define APL_GPIO_NR_DEVICES		4

> +#define APL_GPIO_IRQ			14

> +

> +static struct resource apl_gpio_resources[APL_GPIO_NR_DEVICES][2] = {

> +	{

> +		DEFINE_RES_MEM(APL_GPIO_NORTH_OFFSET,

> APL_GPIO_NORTH_SIZE),

> +		DEFINE_RES_IRQ(APL_GPIO_IRQ),

> +	},

> +	{

> +		DEFINE_RES_MEM(APL_GPIO_NORTHWEST_OFFSET,

> APL_GPIO_NORTHWEST_SIZE),

> +		DEFINE_RES_IRQ(APL_GPIO_IRQ),

> +	},

> +	{

> +		DEFINE_RES_MEM(APL_GPIO_WEST_OFFSET,

> APL_GPIO_WEST_SIZE),

> +		DEFINE_RES_IRQ(APL_GPIO_IRQ),

> +	},

> +	{

> +		DEFINE_RES_MEM(APL_GPIO_SOUTHWEST_OFFSET,

> APL_GPIO_SOUTHWEST_SIZE),

> +		DEFINE_RES_IRQ(APL_GPIO_IRQ),

> +	},

> +};

> +

> +/* The order must be in sync with apl_pinctrl_soc_data */

> +static const struct mfd_cell apl_gpio_devices[APL_GPIO_NR_DEVICES] =

> {

> +	{

> +		/* North */

> +		.name = "apollolake-pinctrl",

> +		.id = 0,

> +		.num_resources = ARRAY_SIZE(apl_gpio_resources[0]),

> +		.resources = apl_gpio_resources[0],

> +		.ignore_resource_conflicts = true,

> +	},

> +	{

> +		/* NorthWest */

> +		.name = "apollolake-pinctrl",

> +		.id = 1,

> +		.num_resources = ARRAY_SIZE(apl_gpio_resources[1]),

> +		.resources = apl_gpio_resources[1],

> +		.ignore_resource_conflicts = true,

> +	},

> +	{

> +		/* West */

> +		.name = "apollolake-pinctrl",

> +		.id = 2,

> +		.num_resources = ARRAY_SIZE(apl_gpio_resources[2]),

> +		.resources = apl_gpio_resources[2],

> +		.ignore_resource_conflicts = true,

> +	},

> +	{

> +		/* SouthWest */

> +		.name = "apollolake-pinctrl",

> +		.id = 3,

> +		.num_resources = ARRAY_SIZE(apl_gpio_resources[3]),

> +		.resources = apl_gpio_resources[3],

> +		.ignore_resource_conflicts = true,

> +	},

> +};

>  

>  static struct mfd_cell lpc_ich_spi_cell = {

>  	.name = "intel-spi",

> @@ -1082,6 +1151,29 @@ static int lpc_ich_init_wdt(struct pci_dev

> *dev) return ret;

>  }

>  

> +static int lpc_ich_init_pinctrl(struct pci_dev *dev)

> +{

> +	struct resource base;

> +	unsigned int i;

> +	int ret;

> +

> +	ret = pci_p2sb_bar(dev, PCI_DEVFN(13, 0), &base);

> +	if (ret)

> +		return ret;

> +

> +	for (i = 0; i < ARRAY_SIZE(apl_gpio_devices); i++) {

> +		struct resource *mem = &apl_gpio_resources[i][0];

> +

> +		/* Fill MEM resource */

> +		mem->start += base.start;

> +		mem->end += base.start;

> +		mem->flags = base.flags;

> +	}

> +

> +	return mfd_add_devices(&dev->dev, 0, apl_gpio_devices,

> +			       ARRAY_SIZE(apl_gpio_devices), NULL,

> 0, NULL); +}

> +

>  static void lpc_ich_test_spi_write(struct pci_dev *dev, unsigned int

> devfn, struct intel_spi_boardinfo *info)

>  {

> @@ -1198,6 +1290,12 @@ static int lpc_ich_probe(struct pci_dev *dev,

>  			cell_added = true;

>  	}

>  

> +	if (priv->chipset == LPC_APL) {

> +		ret = lpc_ich_init_pinctrl(dev);

> +		if (!ret)

> +			cell_added = true;

> +	}

> +

>  	if (lpc_chipset_info[priv->chipset].spi_type) {

>  		ret = lpc_ich_init_spi(dev);

>  		if (!ret)
Henning Schild April 12, 2021, 4:40 p.m. | #3
Tan or Andy,

maybe you can point me to a user of that patch. I guess there might be
an out-of-tree driver or userland code on how to use the GPIOs from
there.

Feel free to send directly to me in case it is not published anywhere
and should not yet be on the list, i could just use it for inspiration.
A driver will likely be GPL anyways.

regards,
Henning

Am Mon, 12 Apr 2021 18:01:06 +0200
schrieb Henning Schild <henning.schild@siemens.com>:

> Am Mon, 8 Mar 2021 14:20:19 +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>

> > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

> > ---

> >  drivers/mfd/lpc_ich.c | 100

> > +++++++++++++++++++++++++++++++++++++++++- 1 file changed, 99

> > insertions(+), 1 deletion(-)

> > 

> > diff --git a/drivers/mfd/lpc_ich.c b/drivers/mfd/lpc_ich.c

> > index 8e9bd6813287..959247b6987a 100644

> > --- a/drivers/mfd/lpc_ich.c

> > +++ b/drivers/mfd/lpc_ich.c

> > @@ -8,7 +8,8 @@

> >   *  Configuration Registers.

> >   *

> >   *  This driver is derived from lpc_sch.

> > -

> > + *

> > + *  Copyright (C) 2017, 2021 Intel Corporation

> >   *  Copyright (c) 2011 Extreme Engineering Solution, Inc.

> >   *  Author: Aaron Sierra <asierra@xes-inc.com>

> >   *

> > @@ -43,6 +44,7 @@

> >  #include <linux/acpi.h>

> >  #include <linux/pci.h>

> >  #include <linux/pci-p2sb.h>

> > +#include <linux/pinctrl/pinctrl.h>

> >  #include <linux/mfd/core.h>

> >  #include <linux/mfd/lpc_ich.h>

> >  #include <linux/platform_data/itco_wdt.h>

> > @@ -140,6 +142,73 @@ static struct mfd_cell lpc_ich_gpio_cell = {

> >  	.ignore_resource_conflicts = true,

> >  };

> >  

> > +/* Offset data for Apollo Lake GPIO controllers */

> > +#define APL_GPIO_SOUTHWEST_OFFSET	0xc00000

> > +#define APL_GPIO_SOUTHWEST_SIZE		0x654

> > +#define APL_GPIO_NORTHWEST_OFFSET	0xc40000

> > +#define APL_GPIO_NORTHWEST_SIZE		0x764

> > +#define APL_GPIO_NORTH_OFFSET		0xc50000

> > +#define APL_GPIO_NORTH_SIZE		0x76c  

> 

> drivers/pinctrl/intel/pinctrl-broxton.c:653

> BXT_COMMUNITY(0, 77),

> 

> > +#define APL_GPIO_WEST_OFFSET		0xc70000

> > +#define APL_GPIO_WEST_SIZE		0x674  

> 

> All these sizes correlate with 4 magic numbers from pinctrl-broxton.

> 

> SIZE - 0x500 (pad_base?) - 4 (no clue) / 8

> 

> It might be worth basing both numbers on a define and giving the magic

> numbers some names.

> 

> But all this seems like duplication of pinctrl-broxton, maybe the

> pinctrl driver should unhide the p2sb ...

> 

> regards,

> Henning

> 

> > +

> > +#define APL_GPIO_NR_DEVICES		4

> > +#define APL_GPIO_IRQ			14

> > +

> > +static struct resource apl_gpio_resources[APL_GPIO_NR_DEVICES][2]

> > = {

> > +	{

> > +		DEFINE_RES_MEM(APL_GPIO_NORTH_OFFSET,

> > APL_GPIO_NORTH_SIZE),

> > +		DEFINE_RES_IRQ(APL_GPIO_IRQ),

> > +	},

> > +	{

> > +		DEFINE_RES_MEM(APL_GPIO_NORTHWEST_OFFSET,

> > APL_GPIO_NORTHWEST_SIZE),

> > +		DEFINE_RES_IRQ(APL_GPIO_IRQ),

> > +	},

> > +	{

> > +		DEFINE_RES_MEM(APL_GPIO_WEST_OFFSET,

> > APL_GPIO_WEST_SIZE),

> > +		DEFINE_RES_IRQ(APL_GPIO_IRQ),

> > +	},

> > +	{

> > +		DEFINE_RES_MEM(APL_GPIO_SOUTHWEST_OFFSET,

> > APL_GPIO_SOUTHWEST_SIZE),

> > +		DEFINE_RES_IRQ(APL_GPIO_IRQ),

> > +	},

> > +};

> > +

> > +/* The order must be in sync with apl_pinctrl_soc_data */

> > +static const struct mfd_cell apl_gpio_devices[APL_GPIO_NR_DEVICES]

> > = {

> > +	{

> > +		/* North */

> > +		.name = "apollolake-pinctrl",

> > +		.id = 0,

> > +		.num_resources = ARRAY_SIZE(apl_gpio_resources[0]),

> > +		.resources = apl_gpio_resources[0],

> > +		.ignore_resource_conflicts = true,

> > +	},

> > +	{

> > +		/* NorthWest */

> > +		.name = "apollolake-pinctrl",

> > +		.id = 1,

> > +		.num_resources = ARRAY_SIZE(apl_gpio_resources[1]),

> > +		.resources = apl_gpio_resources[1],

> > +		.ignore_resource_conflicts = true,

> > +	},

> > +	{

> > +		/* West */

> > +		.name = "apollolake-pinctrl",

> > +		.id = 2,

> > +		.num_resources = ARRAY_SIZE(apl_gpio_resources[2]),

> > +		.resources = apl_gpio_resources[2],

> > +		.ignore_resource_conflicts = true,

> > +	},

> > +	{

> > +		/* SouthWest */

> > +		.name = "apollolake-pinctrl",

> > +		.id = 3,

> > +		.num_resources = ARRAY_SIZE(apl_gpio_resources[3]),

> > +		.resources = apl_gpio_resources[3],

> > +		.ignore_resource_conflicts = true,

> > +	},

> > +};

> >  

> >  static struct mfd_cell lpc_ich_spi_cell = {

> >  	.name = "intel-spi",

> > @@ -1082,6 +1151,29 @@ static int lpc_ich_init_wdt(struct pci_dev

> > *dev) return ret;

> >  }

> >  

> > +static int lpc_ich_init_pinctrl(struct pci_dev *dev)

> > +{

> > +	struct resource base;

> > +	unsigned int i;

> > +	int ret;

> > +

> > +	ret = pci_p2sb_bar(dev, PCI_DEVFN(13, 0), &base);

> > +	if (ret)

> > +		return ret;

> > +

> > +	for (i = 0; i < ARRAY_SIZE(apl_gpio_devices); i++) {

> > +		struct resource *mem = &apl_gpio_resources[i][0];

> > +

> > +		/* Fill MEM resource */

> > +		mem->start += base.start;

> > +		mem->end += base.start;

> > +		mem->flags = base.flags;

> > +	}

> > +

> > +	return mfd_add_devices(&dev->dev, 0, apl_gpio_devices,

> > +			       ARRAY_SIZE(apl_gpio_devices), NULL,

> > 0, NULL); +}

> > +

> >  static void lpc_ich_test_spi_write(struct pci_dev *dev, unsigned

> > int devfn, struct intel_spi_boardinfo *info)

> >  {

> > @@ -1198,6 +1290,12 @@ static int lpc_ich_probe(struct pci_dev *dev,

> >  			cell_added = true;

> >  	}

> >  

> > +	if (priv->chipset == LPC_APL) {

> > +		ret = lpc_ich_init_pinctrl(dev);

> > +		if (!ret)

> > +			cell_added = true;

> > +	}

> > +

> >  	if (lpc_chipset_info[priv->chipset].spi_type) {

> >  		ret = lpc_ich_init_spi(dev);

> >  		if (!ret)  

>
Andy Shevchenko April 12, 2021, 4:51 p.m. | #4
On Mon, Apr 12, 2021 at 06:01:06PM +0200, Henning Schild wrote:
> Am Mon, 8 Mar 2021 14:20:19 +0200

> schrieb Andy Shevchenko <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.


...

> > +/* Offset data for Apollo Lake GPIO controllers */

> > +#define APL_GPIO_SOUTHWEST_OFFSET	0xc00000

> > +#define APL_GPIO_SOUTHWEST_SIZE		0x654

> > +#define APL_GPIO_NORTHWEST_OFFSET	0xc40000

> > +#define APL_GPIO_NORTHWEST_SIZE		0x764

> > +#define APL_GPIO_NORTH_OFFSET		0xc50000

> > +#define APL_GPIO_NORTH_SIZE		0x76c

> 

> drivers/pinctrl/intel/pinctrl-broxton.c:653

> BXT_COMMUNITY(0, 77),

> 

> > +#define APL_GPIO_WEST_OFFSET		0xc70000

> > +#define APL_GPIO_WEST_SIZE		0x674

> 

> All these sizes correlate with 4 magic numbers from pinctrl-broxton.

> 

> SIZE - 0x500 (pad_base?) - 4 (no clue) / 8

> 

> It might be worth basing both numbers on a define and giving the magic

> numbers some names.


I didn't get this, sorry. The numbers above just precise sizes of the
resources. Actually they all one page anyway, so, I can drop magic of SIZEs and
leave only offsets.

> But all this seems like duplication of pinctrl-broxton, maybe the

> pinctrl driver should unhide the p2sb ...


Definitely should not. It's not a business of the pin control driver to know
how it has to be instantiated (or from what data). These offsets belong to the
platform description and since firmware hides the device without given an
appropriate ACPI device node, we have only one choice (assuming firmware is
carved in stone) -- board files.

P2SB on the other hand is a slice of many (independent) devices. There is no
"proper" place to unhide it except some core part of x86 / PCI.


-- 
With Best Regards,
Andy Shevchenko
Andy Shevchenko April 12, 2021, 4:59 p.m. | #5
On Mon, Apr 12, 2021 at 06:40:01PM +0200, Henning Schild wrote:
> Tan or Andy,

> 

> maybe you can point me to a user of that patch. I guess there might be

> an out-of-tree driver or userland code on how to use the GPIOs from

> there.


I'm confused. User of this patch is pinctrl-broxton driver.
It's in upstream.

Using GPIOs from it is something as done in a few drivers already
(Assuming we have no resources described in the ACPI). I.e. you need to
register in board file the GPIO mapping table with help of
devm_acpi_dev_add_driver_gpios() and use one of gpiod_get() family of functions
to request it.

In case of LEDs you simple describe GPIO device name in lookup table and
that's it. The drivers/platform/x86/pcengines-apuv2.c not the best but
will give you an idea how to use "leds-gpio" driver in board files.


> Feel free to send directly to me in case it is not published anywhere

> and should not yet be on the list, i could just use it for inspiration.

> A driver will likely be GPL anyways.


-- 
With Best Regards,
Andy Shevchenko
Henning Schild April 12, 2021, 5:16 p.m. | #6
Am Mon, 12 Apr 2021 19:59:05 +0300
schrieb Andy Shevchenko <andriy.shevchenko@linux.intel.com>:

> On Mon, Apr 12, 2021 at 06:40:01PM +0200, Henning Schild wrote:

> > Tan or Andy,

> > 

> > maybe you can point me to a user of that patch. I guess there might

> > be an out-of-tree driver or userland code on how to use the GPIOs

> > from there.  

> 

> I'm confused. User of this patch is pinctrl-broxton driver.

> It's in upstream.


Should this appear in /sys/class/gpio as chip so that pins can be
exported?

That is what i tried and failed with.

> Using GPIOs from it is something as done in a few drivers already

> (Assuming we have no resources described in the ACPI). I.e. you need

> to register in board file the GPIO mapping table with help of

> devm_acpi_dev_add_driver_gpios() and use one of gpiod_get() family of

> functions to request it.

> 

> In case of LEDs you simple describe GPIO device name in lookup table

> and that's it. The drivers/platform/x86/pcengines-apuv2.c not the

> best but will give you an idea how to use "leds-gpio" driver in board

> files.


I am aware of that driver and had a look at it. In order to figure out
the arguments for the macros/functions i was hoping for userland gpio
"export", but maybe that does not work here ...
For now i will assume that it does not show up in sysfs and can maybe
still be used, and try to build on top.

regards,
Henning

> 

> > Feel free to send directly to me in case it is not published

> > anywhere and should not yet be on the list, i could just use it for

> > inspiration. A driver will likely be GPL anyways.  

>
Henning Schild April 12, 2021, 5:27 p.m. | #7
Am Mon, 12 Apr 2021 19:51:42 +0300
schrieb Andy Shevchenko <andriy.shevchenko@linux.intel.com>:

> On Mon, Apr 12, 2021 at 06:01:06PM +0200, Henning Schild wrote:

> > Am Mon, 8 Mar 2021 14:20:19 +0200

> > schrieb Andy Shevchenko <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.  

> 

> ...

> 

> > > +/* Offset data for Apollo Lake GPIO controllers */

> > > +#define APL_GPIO_SOUTHWEST_OFFSET	0xc00000

> > > +#define APL_GPIO_SOUTHWEST_SIZE		0x654

> > > +#define APL_GPIO_NORTHWEST_OFFSET	0xc40000

> > > +#define APL_GPIO_NORTHWEST_SIZE		0x764

> > > +#define APL_GPIO_NORTH_OFFSET		0xc50000

> > > +#define APL_GPIO_NORTH_SIZE		0x76c  

> > 

> > drivers/pinctrl/intel/pinctrl-broxton.c:653

> > BXT_COMMUNITY(0, 77),

> >   

> > > +#define APL_GPIO_WEST_OFFSET		0xc70000

> > > +#define APL_GPIO_WEST_SIZE		0x674  

> > 

> > All these sizes correlate with 4 magic numbers from pinctrl-broxton.

> > 

> > SIZE - 0x500 (pad_base?) - 4 (no clue) / 8

> > 

> > It might be worth basing both numbers on a define and giving the

> > magic numbers some names.  

> 

> I didn't get this, sorry. The numbers above just precise sizes of the

> resources. Actually they all one page anyway, so, I can drop magic of

> SIZEs and leave only offsets.


That precise size is also in the broxton driver, i think. Say we did
have

#define BXT_NORTH_COUNT 77
#define PAD_BASE 0x500

in some central place

then we could use

size = 0x500 + 8 * BXT_NORTH_COUNT + 4 (no clue what that is)

the same pattern would work for all those sizes and their
BXT_COMMUNITY(0, XX) counterparts

So the real size seems to be a function of the magic numbers in
BXT_COMMUNITY(0, XX)

Or simply take one page as you say.

> > But all this seems like duplication of pinctrl-broxton, maybe the

> > pinctrl driver should unhide the p2sb ...  

> 

> Definitely should not. It's not a business of the pin control driver

> to know how it has to be instantiated (or from what data). These

> offsets belong to the platform description and since firmware hides

> the device without given an appropriate ACPI device node, we have

> only one choice (assuming firmware is carved in stone) -- board files.

> 

> P2SB on the other hand is a slice of many (independent) devices.

> There is no "proper" place to unhide it except some core part of x86

> / PCI.


Got it, still the fact that there are 4 regions/communities is also part
of the broxton driver so there is duplication.

regards,
Henning
Andy Shevchenko April 12, 2021, 5:34 p.m. | #8
On Mon, Apr 12, 2021 at 07:16:53PM +0200, Henning Schild wrote:
> Am Mon, 12 Apr 2021 19:59:05 +0300

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

> > On Mon, Apr 12, 2021 at 06:40:01PM +0200, Henning Schild wrote:

> > > Tan or Andy,

> > > 

> > > maybe you can point me to a user of that patch. I guess there might

> > > be an out-of-tree driver or userland code on how to use the GPIOs

> > > from there.  

> > 

> > I'm confused. User of this patch is pinctrl-broxton driver.

> > It's in upstream.

> 

> Should this appear in /sys/class/gpio as chip so that pins can be

> exported?


No. Sysfs interface is deprecated. It should appear as /dev/gpiochip0 or so.

> That is what i tried and failed with.

> 

> > Using GPIOs from it is something as done in a few drivers already

> > (Assuming we have no resources described in the ACPI). I.e. you need

> > to register in board file the GPIO mapping table with help of

> > devm_acpi_dev_add_driver_gpios() and use one of gpiod_get() family of

> > functions to request it.

> > 

> > In case of LEDs you simple describe GPIO device name in lookup table

> > and that's it. The drivers/platform/x86/pcengines-apuv2.c not the

> > best but will give you an idea how to use "leds-gpio" driver in board

> > files.

> 

> I am aware of that driver and had a look at it. In order to figure out

> the arguments for the macros/functions i was hoping for userland gpio

> "export", but maybe that does not work here ...

> For now i will assume that it does not show up in sysfs and can maybe

> still be used, and try to build on top.


Just switch to use libgpiod and associated tools / bindings in user space.
Sysfs ABI is not being developed anymore.

-- 
With Best Regards,
Andy Shevchenko
Andy Shevchenko April 12, 2021, 5:41 p.m. | #9
On Mon, Apr 12, 2021 at 07:27:14PM +0200, Henning Schild wrote:
> Am Mon, 12 Apr 2021 19:51:42 +0300

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

> > On Mon, Apr 12, 2021 at 06:01:06PM +0200, Henning Schild wrote:

> > > Am Mon, 8 Mar 2021 14:20:19 +0200

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


...

> > > > +#define APL_GPIO_NORTH_OFFSET		0xc50000

> > > > +#define APL_GPIO_NORTH_SIZE		0x76c  

> > > 

> > > drivers/pinctrl/intel/pinctrl-broxton.c:653

> > > BXT_COMMUNITY(0, 77),

> > >   

> > > > +#define APL_GPIO_WEST_OFFSET		0xc70000

> > > > +#define APL_GPIO_WEST_SIZE		0x674  

> > > 

> > > All these sizes correlate with 4 magic numbers from pinctrl-broxton.

> > > 

> > > SIZE - 0x500 (pad_base?) - 4 (no clue) / 8

> > > 

> > > It might be worth basing both numbers on a define and giving the

> > > magic numbers some names.  

> > 

> > I didn't get this, sorry. The numbers above just precise sizes of the

> > resources. Actually they all one page anyway, so, I can drop magic of

> > SIZEs and leave only offsets.

> 

> That precise size is also in the broxton driver, i think. Say we did

> have

> 

> #define BXT_NORTH_COUNT 77

> #define PAD_BASE 0x500

> 

> in some central place

> 

> then we could use

> 

> size = 0x500 + 8 * BXT_NORTH_COUNT + 4 (no clue what that is)

> 

> the same pattern would work for all those sizes and their

> BXT_COMMUNITY(0, XX) counterparts

> 

> So the real size seems to be a function of the magic numbers in

> BXT_COMMUNITY(0, XX)

> 

> Or simply take one page as you say.


No, not this way. We are really trying hard *not* to put *that* magic into
the code. Just FYI that SIZEs I have calculated myself, but these SIZEs
are *not* the same as the ones used in pinctrl-broxton *semantically*.

One if for resource provider, one is for consumer. They are simply different
in this sense.

> > > But all this seems like duplication of pinctrl-broxton, maybe the

> > > pinctrl driver should unhide the p2sb ...  

> > 

> > Definitely should not. It's not a business of the pin control driver

> > to know how it has to be instantiated (or from what data). These

> > offsets belong to the platform description and since firmware hides

> > the device without given an appropriate ACPI device node, we have

> > only one choice (assuming firmware is carved in stone) -- board files.

> > 

> > P2SB on the other hand is a slice of many (independent) devices.

> > There is no "proper" place to unhide it except some core part of x86

> > / PCI.

> 

> Got it, still the fact that there are 4 regions/communities is also part

> of the broxton driver so there is duplication.


See above. I guess here is a misunderstanding behind meaning of the (same)
numbers in different parts. Technically we may unify them, but it will be
a layering violation.

-- 
With Best Regards,
Andy Shevchenko
Henning Schild April 13, 2021, 6:47 a.m. | #10
Am Mon, 12 Apr 2021 20:34:45 +0300
schrieb Andy Shevchenko <andriy.shevchenko@linux.intel.com>:

> On Mon, Apr 12, 2021 at 07:16:53PM +0200, Henning Schild wrote:

> > Am Mon, 12 Apr 2021 19:59:05 +0300

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

> > > On Mon, Apr 12, 2021 at 06:40:01PM +0200, Henning Schild wrote:  

> > > > Tan or Andy,

> > > > 

> > > > maybe you can point me to a user of that patch. I guess there

> > > > might be an out-of-tree driver or userland code on how to use

> > > > the GPIOs from there.    

> > > 

> > > I'm confused. User of this patch is pinctrl-broxton driver.

> > > It's in upstream.  

> > 

> > Should this appear in /sys/class/gpio as chip so that pins can be

> > exported?  

> 

> No. Sysfs interface is deprecated. It should appear as /dev/gpiochip0

> or so.


Ok, just found that there is a null pointer deref in the probe function
of the pinctrl driver, looking into that.

Meanwhile i think i will need a similar patch for
pinctrl-sunrisepoint.c for that wdt, do you happen to have that as
well? Or a spec where to find all the magic numbers.

regards,
Henning

> 

> > That is what i tried and failed with.

> >   

> > > Using GPIOs from it is something as done in a few drivers already

> > > (Assuming we have no resources described in the ACPI). I.e. you

> > > need to register in board file the GPIO mapping table with help of

> > > devm_acpi_dev_add_driver_gpios() and use one of gpiod_get()

> > > family of functions to request it.

> > > 

> > > In case of LEDs you simple describe GPIO device name in lookup

> > > table and that's it. The drivers/platform/x86/pcengines-apuv2.c

> > > not the best but will give you an idea how to use "leds-gpio"

> > > driver in board files.  

> > 

> > I am aware of that driver and had a look at it. In order to figure

> > out the arguments for the macros/functions i was hoping for

> > userland gpio "export", but maybe that does not work here ...

> > For now i will assume that it does not show up in sysfs and can

> > maybe still be used, and try to build on top.  

> 

> Just switch to use libgpiod and associated tools / bindings in user

> space. Sysfs ABI is not being developed anymore.

>

Patch

diff --git a/drivers/mfd/lpc_ich.c b/drivers/mfd/lpc_ich.c
index 8e9bd6813287..959247b6987a 100644
--- a/drivers/mfd/lpc_ich.c
+++ b/drivers/mfd/lpc_ich.c
@@ -8,7 +8,8 @@ 
  *  Configuration Registers.
  *
  *  This driver is derived from lpc_sch.
-
+ *
+ *  Copyright (C) 2017, 2021 Intel Corporation
  *  Copyright (c) 2011 Extreme Engineering Solution, Inc.
  *  Author: Aaron Sierra <asierra@xes-inc.com>
  *
@@ -43,6 +44,7 @@ 
 #include <linux/acpi.h>
 #include <linux/pci.h>
 #include <linux/pci-p2sb.h>
+#include <linux/pinctrl/pinctrl.h>
 #include <linux/mfd/core.h>
 #include <linux/mfd/lpc_ich.h>
 #include <linux/platform_data/itco_wdt.h>
@@ -140,6 +142,73 @@  static struct mfd_cell lpc_ich_gpio_cell = {
 	.ignore_resource_conflicts = true,
 };
 
+/* Offset data for Apollo Lake GPIO controllers */
+#define APL_GPIO_SOUTHWEST_OFFSET	0xc00000
+#define APL_GPIO_SOUTHWEST_SIZE		0x654
+#define APL_GPIO_NORTHWEST_OFFSET	0xc40000
+#define APL_GPIO_NORTHWEST_SIZE		0x764
+#define APL_GPIO_NORTH_OFFSET		0xc50000
+#define APL_GPIO_NORTH_SIZE		0x76c
+#define APL_GPIO_WEST_OFFSET		0xc70000
+#define APL_GPIO_WEST_SIZE		0x674
+
+#define APL_GPIO_NR_DEVICES		4
+#define APL_GPIO_IRQ			14
+
+static struct resource apl_gpio_resources[APL_GPIO_NR_DEVICES][2] = {
+	{
+		DEFINE_RES_MEM(APL_GPIO_NORTH_OFFSET, APL_GPIO_NORTH_SIZE),
+		DEFINE_RES_IRQ(APL_GPIO_IRQ),
+	},
+	{
+		DEFINE_RES_MEM(APL_GPIO_NORTHWEST_OFFSET, APL_GPIO_NORTHWEST_SIZE),
+		DEFINE_RES_IRQ(APL_GPIO_IRQ),
+	},
+	{
+		DEFINE_RES_MEM(APL_GPIO_WEST_OFFSET, APL_GPIO_WEST_SIZE),
+		DEFINE_RES_IRQ(APL_GPIO_IRQ),
+	},
+	{
+		DEFINE_RES_MEM(APL_GPIO_SOUTHWEST_OFFSET, APL_GPIO_SOUTHWEST_SIZE),
+		DEFINE_RES_IRQ(APL_GPIO_IRQ),
+	},
+};
+
+/* The order must be in sync with apl_pinctrl_soc_data */
+static const struct mfd_cell apl_gpio_devices[APL_GPIO_NR_DEVICES] = {
+	{
+		/* North */
+		.name = "apollolake-pinctrl",
+		.id = 0,
+		.num_resources = ARRAY_SIZE(apl_gpio_resources[0]),
+		.resources = apl_gpio_resources[0],
+		.ignore_resource_conflicts = true,
+	},
+	{
+		/* NorthWest */
+		.name = "apollolake-pinctrl",
+		.id = 1,
+		.num_resources = ARRAY_SIZE(apl_gpio_resources[1]),
+		.resources = apl_gpio_resources[1],
+		.ignore_resource_conflicts = true,
+	},
+	{
+		/* West */
+		.name = "apollolake-pinctrl",
+		.id = 2,
+		.num_resources = ARRAY_SIZE(apl_gpio_resources[2]),
+		.resources = apl_gpio_resources[2],
+		.ignore_resource_conflicts = true,
+	},
+	{
+		/* SouthWest */
+		.name = "apollolake-pinctrl",
+		.id = 3,
+		.num_resources = ARRAY_SIZE(apl_gpio_resources[3]),
+		.resources = apl_gpio_resources[3],
+		.ignore_resource_conflicts = true,
+	},
+};
 
 static struct mfd_cell lpc_ich_spi_cell = {
 	.name = "intel-spi",
@@ -1082,6 +1151,29 @@  static int lpc_ich_init_wdt(struct pci_dev *dev)
 	return ret;
 }
 
+static int lpc_ich_init_pinctrl(struct pci_dev *dev)
+{
+	struct resource base;
+	unsigned int i;
+	int ret;
+
+	ret = pci_p2sb_bar(dev, PCI_DEVFN(13, 0), &base);
+	if (ret)
+		return ret;
+
+	for (i = 0; i < ARRAY_SIZE(apl_gpio_devices); i++) {
+		struct resource *mem = &apl_gpio_resources[i][0];
+
+		/* Fill MEM resource */
+		mem->start += base.start;
+		mem->end += base.start;
+		mem->flags = base.flags;
+	}
+
+	return mfd_add_devices(&dev->dev, 0, apl_gpio_devices,
+			       ARRAY_SIZE(apl_gpio_devices), NULL, 0, NULL);
+}
+
 static void lpc_ich_test_spi_write(struct pci_dev *dev, unsigned int devfn,
 				   struct intel_spi_boardinfo *info)
 {
@@ -1198,6 +1290,12 @@  static int lpc_ich_probe(struct pci_dev *dev,
 			cell_added = true;
 	}
 
+	if (priv->chipset == LPC_APL) {
+		ret = lpc_ich_init_pinctrl(dev);
+		if (!ret)
+			cell_added = true;
+	}
+
 	if (lpc_chipset_info[priv->chipset].spi_type) {
 		ret = lpc_ich_init_spi(dev);
 		if (!ret)