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

["Tan, Jui Nee" <jui.nee.tan@xxxxxxxxx>]

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

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

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

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

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

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

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

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

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

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

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

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