> -----Original Message-----
> From: Lee Jones [mailto:lee.jo...@linaro.org]
> Sent: Monday, May 9, 2016 8:25 PM
> To: Tan, Jui Nee <jui.nee....@intel.com>
> Cc: mika.westerb...@linux.intel.com; heikki.kroge...@linux.intel.com;
> andriy.shevche...@linux.intel.com; t...@linutronix.de;
> mi...@redhat.com; h...@zytor.com; x...@kernel.org; pty...@xes-inc.com;
> linux-g...@vger.kernel.org; linux-kernel@vger.kernel.org; Yong, Jonathan
> <jonathan.y...@intel.com>; Yu, Ong Hock <ong.hock...@intel.com>; Voon,
> Weifeng <weifeng.v...@intel.com>; Wan Mohamad, Wan Ahmad Zainie
> <wan.ahmad.zainie.wan.moha...@intel.com>
> 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....@intel.com>
> > ---
> > 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
