| Message ID | 20210308122020.57071-7-andriy.shevchenko@linux.intel.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
| Series | PCI: introduce p2sb helper | expand |
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)
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)
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) >
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.
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.
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. >
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
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.
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.
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. >
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)