There are a few users and at least one more is coming (*) that would like
to utilize P2SB mechanism of hiding and unhiding a device from the PCI
configuration space.
Here is the series to consolidate p2sb handling code for existing users
and provide a generic way for new comer(s).
It also includes a patch to enable GPIO controllers on Apollo Lake
when it's used with ABL bootloader w/o ACPI support.
The patch that bring the helper ("platform/x86/intel: Add Primary
to Sideband (P2SB) bridge support") has a commit message that
sheds a light on what the P2SB is and why this is needed.
The changes made in v2 do not change the main idea and the functionality
in a big scale. What we need is probably one more (RE-)test done by Henning.
I hope to have it merged to v5.18-rc1 that Siemens can develop their changes
based on this series.
I have tested this on Apollo Lake platform (I'm able to see SPI NOR and
since we have an ACPI device for GPIO I do not see any attempts to recreate
one).
*) One in this series, and one is a due after merge in the Simatic IPC drivers
The series may be routed either via MFD (and I guess Lee would prefer that)
or via PDx86, whichever seems better for you, folks. As of today patches
are ACKed by the respective maintainers, but I2C one and one of the MFD.
Wolfram, can you ACK the patch against i2c-i801 driver, if you have no
objections?
Changes in v4:
- added tag to the entire series (Hans)
- added tag to pin control patch (Mika)
- dropped PCI core changes (PCI core doesn't want modifications to be made)
- as a consequence of the above merged necessary bits into p2sb.c
- added a check that p2sb is really hidden (Hans)
- added EDAC patches (reviewed by maintainer internally)
Changes in v3:
- resent with cover letter
Changes in v2:
- added parentheses around bus in macros (Joe)
- added tag (Jean)
- fixed indentation and wrapping in the header (Christoph)
- moved out of PCI realm to PDx86 as the best common denominator (Bjorn)
- added a verbose commit message to explain P2SB thingy (Bjorn)
- converted first parameter from pci_dev to pci_bus
- made first two parameters (bus and devfn) optional (Henning, Lee)
- added Intel pin control patch to the series (Henning, Mika)
- fixed English style in the commit message of one of MFD patch (Lee)
- added tags to my MFD LPC ICH patches (Lee)
- used consistently (c) (Lee)
- made indexing for MFD cell and resource arrays (Lee)
- fixed the resource size in i801 (Jean)
Andy Shevchenko (6):
pinctrl: intel: Check against matching data instead of ACPI companion
mfd: lpc_ich: Factor out lpc_ich_enable_spi_write()
mfd: lpc_ich: Switch to generic p2sb_bar()
i2c: i801: convert to use common P2SB accessor
EDAC, pnd2: Use proper I/O accessors and address space annotation
EDAC, pnd2: convert to use common P2SB accessor
Jonathan Yong (1):
platform/x86/intel: Add Primary to Sideband (P2SB) bridge support
Tan Jui Nee (1):
mfd: lpc_ich: Add support for pinctrl in non-ACPI system
drivers/edac/Kconfig | 1 +
drivers/edac/pnd2_edac.c | 62 ++---
drivers/i2c/busses/Kconfig | 1 +
drivers/i2c/busses/i2c-i801.c | 39 +---
drivers/mfd/Kconfig | 1 +
drivers/mfd/lpc_ich.c | 136 +++++++++--
drivers/pinctrl/intel/pinctrl-intel.c | 14 +-
drivers/platform/x86/intel/Kconfig | 12 +
drivers/platform/x86/intel/Makefile | 1 +
drivers/platform/x86/intel/p2sb.c | 305 +++++++++++++++++++++++++
include/linux/platform_data/x86/p2sb.h | 27 +++
11 files changed, 500 insertions(+), 99 deletions(-)
create mode 100644 drivers/platform/x86/intel/p2sb.c
create mode 100644 include/linux/platform_data/x86/p2sb.h
--
2.34.1
From: Tan Jui Nee <[email protected]>
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 <[email protected]>
Co-developed-by: Andy Shevchenko <[email protected]>
Signed-off-by: Andy Shevchenko <[email protected]>
Acked-by: Hans de Goede <[email protected]>
Acked-by: Linus Walleij <[email protected]>
---
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 <[email protected]>
*
@@ -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)
--
2.34.1
On Mon, 31 Jan 2022, Andy Shevchenko wrote:
> From: Tan Jui Nee <[email protected]>
>
> 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 <[email protected]>
> Co-developed-by: Andy Shevchenko <[email protected]>
> Signed-off-by: Andy Shevchenko <[email protected]>
> Acked-by: Hans de Goede <[email protected]>
> Acked-by: Linus Walleij <[email protected]>
> ---
> 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 <[email protected]>
> *
> @@ -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),
Are these 0x1000's being over-written in lpc_ich_init_pinctrl()?
If so, why pre-initialise?
> + 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 */
Why does the order matter if you've pre-enumerated them all?
> +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)
--
Lee Jones [李琼斯]
Principal Technical Lead - Developer Services
Linaro.org │ Open source software for Arm SoCs
Follow Linaro: Facebook | Twitter | Blog
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!
--
Lee Jones [李琼斯]
Principal Technical Lead - Developer Services
Linaro.org │ Open source software for Arm SoCs
Follow Linaro: Facebook | Twitter | Blog
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.
> > + DEFINE_RES_IRQ(APL_GPIO_IRQ),
> > + },
...
> > +/* The order must be in sync with apl_pinctrl_soc_data */
>
> Why does the order matter if you've pre-enumerated them all?
Indeed. I will drop the confusing comment in the next version.
--
With Best Regards,
Andy Shevchenko
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 <[email protected]>:
> From: Tan Jui Nee <[email protected]>
>
> 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 <[email protected]>
> Co-developed-by: Andy Shevchenko <[email protected]>
> Signed-off-by: Andy Shevchenko <[email protected]>
> Acked-by: Hans de Goede <[email protected]>
> Acked-by: Linus Walleij <[email protected]>
> ---
> 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 <[email protected]>
> *
> @@ -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)
On Mon, Mar 7, 2022 at 8:21 PM Henning Schild
<[email protected]> 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.
--
With Best Regards,
Andy Shevchenko
Am Mon, 31 Jan 2022 17:13:38 +0200
schrieb Andy Shevchenko <[email protected]>:
> There are a few users and at least one more is coming (*) that would
> like to utilize P2SB mechanism of hiding and unhiding a device from
> the PCI configuration space.
>
> Here is the series to consolidate p2sb handling code for existing
> users and provide a generic way for new comer(s).
>
> It also includes a patch to enable GPIO controllers on Apollo Lake
> when it's used with ABL bootloader w/o ACPI support.
>
> The patch that bring the helper ("platform/x86/intel: Add Primary
> to Sideband (P2SB) bridge support") has a commit message that
> sheds a light on what the P2SB is and why this is needed.
>
> The changes made in v2 do not change the main idea and the
> functionality in a big scale. What we need is probably one more
> (RE-)test done by Henning.
Just actually read all this the first time, sorry for that! Will take
care of testing on a few of those Simatic IPCs and write patches to
switch to P2SB ... possibly pinctrl where that might pop up without
ACPI.
I guess p5 will make pinctrl show up on what i would call a 127E ...
apollo lake.
> I hope to have it merged to v5.18-rc1 that
> Siemens can develop their changes based on this series.
Will do.
Sorry for the delay after having passed you i now seem to slow you down.
Henning
> I have tested this on Apollo Lake platform (I'm able to see SPI NOR
> and since we have an ACPI device for GPIO I do not see any attempts
> to recreate one).
>
> *) One in this series, and one is a due after merge in the Simatic
> IPC drivers
>
> The series may be routed either via MFD (and I guess Lee would prefer
> that) or via PDx86, whichever seems better for you, folks. As of
> today patches are ACKed by the respective maintainers, but I2C one
> and one of the MFD.
>
> Wolfram, can you ACK the patch against i2c-i801 driver, if you have no
> objections?
>
> Changes in v4:
> - added tag to the entire series (Hans)
> - added tag to pin control patch (Mika)
> - dropped PCI core changes (PCI core doesn't want modifications to be
> made)
> - as a consequence of the above merged necessary bits into p2sb.c
> - added a check that p2sb is really hidden (Hans)
> - added EDAC patches (reviewed by maintainer internally)
>
> Changes in v3:
> - resent with cover letter
>
> Changes in v2:
> - added parentheses around bus in macros (Joe)
> - added tag (Jean)
> - fixed indentation and wrapping in the header (Christoph)
> - moved out of PCI realm to PDx86 as the best common denominator
> (Bjorn)
> - added a verbose commit message to explain P2SB thingy (Bjorn)
> - converted first parameter from pci_dev to pci_bus
> - made first two parameters (bus and devfn) optional (Henning, Lee)
> - added Intel pin control patch to the series (Henning, Mika)
> - fixed English style in the commit message of one of MFD patch (Lee)
> - added tags to my MFD LPC ICH patches (Lee)
> - used consistently (c) (Lee)
> - made indexing for MFD cell and resource arrays (Lee)
> - fixed the resource size in i801 (Jean)
>
> Andy Shevchenko (6):
> pinctrl: intel: Check against matching data instead of ACPI
> companion mfd: lpc_ich: Factor out lpc_ich_enable_spi_write()
> mfd: lpc_ich: Switch to generic p2sb_bar()
> i2c: i801: convert to use common P2SB accessor
> EDAC, pnd2: Use proper I/O accessors and address space annotation
> EDAC, pnd2: convert to use common P2SB accessor
>
> Jonathan Yong (1):
> platform/x86/intel: Add Primary to Sideband (P2SB) bridge support
>
> Tan Jui Nee (1):
> mfd: lpc_ich: Add support for pinctrl in non-ACPI system
>
> drivers/edac/Kconfig | 1 +
> drivers/edac/pnd2_edac.c | 62 ++---
> drivers/i2c/busses/Kconfig | 1 +
> drivers/i2c/busses/i2c-i801.c | 39 +---
> drivers/mfd/Kconfig | 1 +
> drivers/mfd/lpc_ich.c | 136 +++++++++--
> drivers/pinctrl/intel/pinctrl-intel.c | 14 +-
> drivers/platform/x86/intel/Kconfig | 12 +
> drivers/platform/x86/intel/Makefile | 1 +
> drivers/platform/x86/intel/p2sb.c | 305
> +++++++++++++++++++++++++ include/linux/platform_data/x86/p2sb.h |
> 27 +++ 11 files changed, 500 insertions(+), 99 deletions(-)
> create mode 100644 drivers/platform/x86/intel/p2sb.c
> create mode 100644 include/linux/platform_data/x86/p2sb.h
>
This switches the simatic-ipc modules to using the p2sb interface
introduced by Andy with "platform/x86: introduce p2sb_bar() helper".
It also switches to one apollo lake device to using gpio leds.
I am kind of hoping Andy will take this on top and propose it in his
series.
Henning Schild (2):
simatic-ipc: convert to use common P2SB accessor
leds: simatic-ipc-leds-gpio: add GPIO version of Siemens driver
drivers/leds/simple/Kconfig | 11 ++
drivers/leds/simple/Makefile | 3 +-
drivers/leds/simple/simatic-ipc-leds-gpio.c | 108 ++++++++++++++++++
drivers/leds/simple/simatic-ipc-leds.c | 77 +------------
drivers/platform/x86/simatic-ipc.c | 43 +------
drivers/watchdog/Kconfig | 1 +
drivers/watchdog/simatic-ipc-wdt.c | 15 +--
.../platform_data/x86/simatic-ipc-base.h | 2 -
8 files changed, 139 insertions(+), 121 deletions(-)
create mode 100644 drivers/leds/simple/simatic-ipc-leds-gpio.c
--
2.34.1
Since we have a common P2SB accessor in tree we may use it instead of
open coded variants.
Replace custom code by p2sb_bar() call.
Signed-off-by: Henning Schild <[email protected]>
---
drivers/leds/simple/Kconfig | 1 +
drivers/leds/simple/simatic-ipc-leds.c | 14 +++----
drivers/platform/x86/simatic-ipc.c | 38 -------------------
drivers/watchdog/Kconfig | 1 +
drivers/watchdog/simatic-ipc-wdt.c | 15 ++++----
.../platform_data/x86/simatic-ipc-base.h | 2 -
6 files changed, 17 insertions(+), 54 deletions(-)
diff --git a/drivers/leds/simple/Kconfig b/drivers/leds/simple/Kconfig
index 9f6a68336659..9293e6b36c75 100644
--- a/drivers/leds/simple/Kconfig
+++ b/drivers/leds/simple/Kconfig
@@ -3,6 +3,7 @@ config LEDS_SIEMENS_SIMATIC_IPC
tristate "LED driver for Siemens Simatic IPCs"
depends on LEDS_CLASS
depends on SIEMENS_SIMATIC_IPC
+ select P2SB if X86
help
This option enables support for the LEDs of several Industrial PCs
from Siemens.
diff --git a/drivers/leds/simple/simatic-ipc-leds.c b/drivers/leds/simple/simatic-ipc-leds.c
index ff2c96e73241..215ef5b74236 100644
--- a/drivers/leds/simple/simatic-ipc-leds.c
+++ b/drivers/leds/simple/simatic-ipc-leds.c
@@ -15,6 +15,7 @@
#include <linux/leds.h>
#include <linux/module.h>
#include <linux/pci.h>
+#include <linux/platform_data/x86/p2sb.h>
#include <linux/platform_data/x86/simatic-ipc-base.h>
#include <linux/platform_device.h>
#include <linux/sizes.h>
@@ -38,8 +39,8 @@ static struct simatic_ipc_led simatic_ipc_leds_io[] = {
{ }
};
-/* the actual start will be discovered with PCI, 0 is a placeholder */
-struct resource simatic_ipc_led_mem_res = DEFINE_RES_MEM_NAMED(0, SZ_4K, KBUILD_MODNAME);
+/* the actual start will be discovered with p2sb, 0 is a placeholder */
+struct resource simatic_ipc_led_mem_res = DEFINE_RES_MEM_NAMED(0, 0, KBUILD_MODNAME);
static void *simatic_ipc_led_memory;
@@ -143,14 +144,13 @@ static int simatic_ipc_leds_probe(struct platform_device *pdev)
ipcled = simatic_ipc_leds_mem;
type = IORESOURCE_MEM;
- /* get GPIO base from PCI */
- res->start = simatic_ipc_get_membase0(PCI_DEVFN(13, 0));
- if (res->start == 0)
- return -ENODEV;
+ err = p2sb_bar(NULL, 0, res);
+ if (err)
+ return err;
/* do the final address calculation */
res->start = res->start + (0xC5 << 16);
- res->end += res->start;
+ res->end = res->start + SZ_4K - 1;
simatic_ipc_led_memory = devm_ioremap_resource(dev, res);
if (IS_ERR(simatic_ipc_led_memory))
diff --git a/drivers/platform/x86/simatic-ipc.c b/drivers/platform/x86/simatic-ipc.c
index b599cda5ba3c..26c35e1660cb 100644
--- a/drivers/platform/x86/simatic-ipc.c
+++ b/drivers/platform/x86/simatic-ipc.c
@@ -101,44 +101,6 @@ static int register_platform_devices(u32 station_id)
return 0;
}
-/* FIXME: this should eventually be done with generic P2SB discovery code
- * the individual drivers for watchdogs and LEDs access memory that implements
- * GPIO, but pinctrl will not come up because of missing ACPI entries
- *
- * While there is no conflict a cleaner solution would be to somehow bring up
- * pinctrl even with these ACPI entries missing, and base the drivers on pinctrl.
- * After which the following function could be dropped, together with the code
- * poking the memory.
- */
-/*
- * Get membase address from PCI, used in leds and wdt module. Here we read
- * the bar0. The final address calculation is done in the appropriate modules
- */
-u32 simatic_ipc_get_membase0(unsigned int p2sb)
-{
- struct pci_bus *bus;
- u32 bar0 = 0;
- /*
- * The GPIO memory is in bar0 of the hidden P2SB device.
- * Unhide the device to have a quick look at it, before we hide it
- * again.
- * Also grab the pci rescan lock so that device does not get discovered
- * and remapped while it is visible.
- * This code is inspired by drivers/mfd/lpc_ich.c
- */
- bus = pci_find_bus(0, 0);
- pci_lock_rescan_remove();
- pci_bus_write_config_byte(bus, p2sb, 0xE1, 0x0);
- pci_bus_read_config_dword(bus, p2sb, PCI_BASE_ADDRESS_0, &bar0);
-
- bar0 &= ~0xf;
- pci_bus_write_config_byte(bus, p2sb, 0xE1, 0x1);
- pci_unlock_rescan_remove();
-
- return bar0;
-}
-EXPORT_SYMBOL(simatic_ipc_get_membase0);
-
static int __init simatic_ipc_init_module(void)
{
const struct dmi_system_id *match;
diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
index c8fa79da23b3..ce44a942fc68 100644
--- a/drivers/watchdog/Kconfig
+++ b/drivers/watchdog/Kconfig
@@ -1628,6 +1628,7 @@ config SIEMENS_SIMATIC_IPC_WDT
tristate "Siemens Simatic IPC Watchdog"
depends on SIEMENS_SIMATIC_IPC
select WATCHDOG_CORE
+ select P2SB if X86
help
This driver adds support for several watchdogs found in Industrial
PCs from Siemens.
diff --git a/drivers/watchdog/simatic-ipc-wdt.c b/drivers/watchdog/simatic-ipc-wdt.c
index 8bac793c63fb..6599695dc672 100644
--- a/drivers/watchdog/simatic-ipc-wdt.c
+++ b/drivers/watchdog/simatic-ipc-wdt.c
@@ -16,6 +16,7 @@
#include <linux/kernel.h>
#include <linux/module.h>
#include <linux/pci.h>
+#include <linux/platform_data/x86/p2sb.h>
#include <linux/platform_data/x86/simatic-ipc-base.h>
#include <linux/platform_device.h>
#include <linux/sizes.h>
@@ -54,9 +55,9 @@ static struct resource io_resource_trigger =
DEFINE_RES_IO_NAMED(WD_TRIGGER_IOADR, SZ_1,
KBUILD_MODNAME " WD_TRIGGER_IOADR");
-/* the actual start will be discovered with pci, 0 is a placeholder */
+/* the actual start will be discovered with p2sb, 0 is a placeholder */
static struct resource mem_resource =
- DEFINE_RES_MEM_NAMED(0, SZ_4, "WD_RESET_BASE_ADR");
+ DEFINE_RES_MEM_NAMED(0, 0, "WD_RESET_BASE_ADR");
static u32 wd_timeout_table[] = {2, 4, 6, 8, 16, 32, 48, 64 };
static void __iomem *wd_reset_base_addr;
@@ -150,6 +151,7 @@ static int simatic_ipc_wdt_probe(struct platform_device *pdev)
struct simatic_ipc_platform *plat = pdev->dev.platform_data;
struct device *dev = &pdev->dev;
struct resource *res;
+ int ret;
switch (plat->devmode) {
case SIMATIC_IPC_DEVICE_227E:
@@ -190,15 +192,14 @@ static int simatic_ipc_wdt_probe(struct platform_device *pdev)
if (plat->devmode == SIMATIC_IPC_DEVICE_427E) {
res = &mem_resource;
- /* get GPIO base from PCI */
- res->start = simatic_ipc_get_membase0(PCI_DEVFN(0x1f, 1));
- if (res->start == 0)
- return -ENODEV;
+ ret = p2sb_bar(NULL, 0, res);
+ if (ret)
+ return ret;
/* do the final address calculation */
res->start = res->start + (GPIO_COMMUNITY0_PORT_ID << 16) +
PAD_CFG_DW0_GPP_A_23;
- res->end += res->start;
+ res->end = res->start + SZ_4 - 1;
wd_reset_base_addr = devm_ioremap_resource(dev, res);
if (IS_ERR(wd_reset_base_addr))
diff --git a/include/linux/platform_data/x86/simatic-ipc-base.h b/include/linux/platform_data/x86/simatic-ipc-base.h
index 62d2bc774067..39fefd48cf4d 100644
--- a/include/linux/platform_data/x86/simatic-ipc-base.h
+++ b/include/linux/platform_data/x86/simatic-ipc-base.h
@@ -24,6 +24,4 @@ struct simatic_ipc_platform {
u8 devmode;
};
-u32 simatic_ipc_get_membase0(unsigned int p2sb);
-
#endif /* __PLATFORM_DATA_X86_SIMATIC_IPC_BASE_H */
--
2.34.1
Am Tue, 8 Mar 2022 20:35:21 +0100
schrieb Henning Schild <[email protected]>:
> Since we have a common P2SB accessor in tree we may use it instead of
> open coded variants.
>
> Replace custom code by p2sb_bar() call.
>
> Signed-off-by: Henning Schild <[email protected]>
> ---
> drivers/leds/simple/Kconfig | 1 +
> drivers/leds/simple/simatic-ipc-leds.c | 14 +++----
> drivers/platform/x86/simatic-ipc.c | 38
> ------------------- drivers/watchdog/Kconfig |
> 1 + drivers/watchdog/simatic-ipc-wdt.c | 15 ++++----
> .../platform_data/x86/simatic-ipc-base.h | 2 -
> 6 files changed, 17 insertions(+), 54 deletions(-)
>
> diff --git a/drivers/leds/simple/Kconfig b/drivers/leds/simple/Kconfig
> index 9f6a68336659..9293e6b36c75 100644
> --- a/drivers/leds/simple/Kconfig
> +++ b/drivers/leds/simple/Kconfig
> @@ -3,6 +3,7 @@ config LEDS_SIEMENS_SIMATIC_IPC
> tristate "LED driver for Siemens Simatic IPCs"
> depends on LEDS_CLASS
> depends on SIEMENS_SIMATIC_IPC
> + select P2SB if X86
> help
> This option enables support for the LEDs of several
> Industrial PCs from Siemens.
> diff --git a/drivers/leds/simple/simatic-ipc-leds.c
> b/drivers/leds/simple/simatic-ipc-leds.c index
> ff2c96e73241..215ef5b74236 100644 ---
> a/drivers/leds/simple/simatic-ipc-leds.c +++
> b/drivers/leds/simple/simatic-ipc-leds.c @@ -15,6 +15,7 @@
> #include <linux/leds.h>
> #include <linux/module.h>
> #include <linux/pci.h>
> +#include <linux/platform_data/x86/p2sb.h>
> #include <linux/platform_data/x86/simatic-ipc-base.h>
> #include <linux/platform_device.h>
> #include <linux/sizes.h>
> @@ -38,8 +39,8 @@ static struct simatic_ipc_led simatic_ipc_leds_io[]
> = { { }
> };
>
> -/* the actual start will be discovered with PCI, 0 is a placeholder
> */ -struct resource simatic_ipc_led_mem_res = DEFINE_RES_MEM_NAMED(0,
> SZ_4K, KBUILD_MODNAME); +/* the actual start will be discovered with
> p2sb, 0 is a placeholder */ +struct resource simatic_ipc_led_mem_res
> = DEFINE_RES_MEM_NAMED(0, 0, KBUILD_MODNAME);
> static void *simatic_ipc_led_memory;
>
> @@ -143,14 +144,13 @@ static int simatic_ipc_leds_probe(struct
> platform_device *pdev) ipcled = simatic_ipc_leds_mem;
> type = IORESOURCE_MEM;
>
> - /* get GPIO base from PCI */
> - res->start = simatic_ipc_get_membase0(PCI_DEVFN(13,
> 0));
> - if (res->start == 0)
> - return -ENODEV;
> + err = p2sb_bar(NULL, 0, res);
> + if (err)
> + return err;
>
> /* do the final address calculation */
> res->start = res->start + (0xC5 << 16);
> - res->end += res->start;
> + res->end = res->start + SZ_4K - 1;
>
> simatic_ipc_led_memory = devm_ioremap_resource(dev,
After "mfd: lpc_ich: Add support for pinctrl in non-ACPI system" this
will fail because the region will be claimed by pinctrl, did not check
if it would work with !CONFIG_LPC_SCH.
But not a big deal and was expected to happen. I do have a version that
will devm_ioremap (no region) but that seems too hacky to put here.
After all the next patch will remove all that and switch to GPIO.
If we have to propose things in two series i propose to make
CONFIG_LPC_SCH and CONFIG_LEDS_SIEMENS_SIMATIC_IPC conflicting for
intermediate steps.
regards,
Henning
> res); if (IS_ERR(simatic_ipc_led_memory))
> diff --git a/drivers/platform/x86/simatic-ipc.c
> b/drivers/platform/x86/simatic-ipc.c index b599cda5ba3c..26c35e1660cb
> 100644 --- a/drivers/platform/x86/simatic-ipc.c
> +++ b/drivers/platform/x86/simatic-ipc.c
> @@ -101,44 +101,6 @@ static int register_platform_devices(u32
> station_id) return 0;
> }
>
> -/* FIXME: this should eventually be done with generic P2SB discovery
> code
> - * the individual drivers for watchdogs and LEDs access memory that
> implements
> - * GPIO, but pinctrl will not come up because of missing ACPI entries
> - *
> - * While there is no conflict a cleaner solution would be to somehow
> bring up
> - * pinctrl even with these ACPI entries missing, and base the
> drivers on pinctrl.
> - * After which the following function could be dropped, together
> with the code
> - * poking the memory.
> - */
> -/*
> - * Get membase address from PCI, used in leds and wdt module. Here
> we read
> - * the bar0. The final address calculation is done in the
> appropriate modules
> - */
> -u32 simatic_ipc_get_membase0(unsigned int p2sb)
> -{
> - struct pci_bus *bus;
> - u32 bar0 = 0;
> - /*
> - * The GPIO memory is in bar0 of the hidden P2SB device.
> - * Unhide the device to have a quick look at it, before we
> hide it
> - * again.
> - * Also grab the pci rescan lock so that device does not get
> discovered
> - * and remapped while it is visible.
> - * This code is inspired by drivers/mfd/lpc_ich.c
> - */
> - bus = pci_find_bus(0, 0);
> - pci_lock_rescan_remove();
> - pci_bus_write_config_byte(bus, p2sb, 0xE1, 0x0);
> - pci_bus_read_config_dword(bus, p2sb, PCI_BASE_ADDRESS_0,
> &bar0); -
> - bar0 &= ~0xf;
> - pci_bus_write_config_byte(bus, p2sb, 0xE1, 0x1);
> - pci_unlock_rescan_remove();
> -
> - return bar0;
> -}
> -EXPORT_SYMBOL(simatic_ipc_get_membase0);
> -
> static int __init simatic_ipc_init_module(void)
> {
> const struct dmi_system_id *match;
> diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
> index c8fa79da23b3..ce44a942fc68 100644
> --- a/drivers/watchdog/Kconfig
> +++ b/drivers/watchdog/Kconfig
> @@ -1628,6 +1628,7 @@ config SIEMENS_SIMATIC_IPC_WDT
> tristate "Siemens Simatic IPC Watchdog"
> depends on SIEMENS_SIMATIC_IPC
> select WATCHDOG_CORE
> + select P2SB if X86
> help
> This driver adds support for several watchdogs found in
> Industrial PCs from Siemens.
> diff --git a/drivers/watchdog/simatic-ipc-wdt.c
> b/drivers/watchdog/simatic-ipc-wdt.c index 8bac793c63fb..6599695dc672
> 100644 --- a/drivers/watchdog/simatic-ipc-wdt.c
> +++ b/drivers/watchdog/simatic-ipc-wdt.c
> @@ -16,6 +16,7 @@
> #include <linux/kernel.h>
> #include <linux/module.h>
> #include <linux/pci.h>
> +#include <linux/platform_data/x86/p2sb.h>
> #include <linux/platform_data/x86/simatic-ipc-base.h>
> #include <linux/platform_device.h>
> #include <linux/sizes.h>
> @@ -54,9 +55,9 @@ static struct resource io_resource_trigger =
> DEFINE_RES_IO_NAMED(WD_TRIGGER_IOADR, SZ_1,
> KBUILD_MODNAME " WD_TRIGGER_IOADR");
>
> -/* the actual start will be discovered with pci, 0 is a placeholder
> */ +/* the actual start will be discovered with p2sb, 0 is a
> placeholder */ static struct resource mem_resource =
> - DEFINE_RES_MEM_NAMED(0, SZ_4, "WD_RESET_BASE_ADR");
> + DEFINE_RES_MEM_NAMED(0, 0, "WD_RESET_BASE_ADR");
>
> static u32 wd_timeout_table[] = {2, 4, 6, 8, 16, 32, 48, 64 };
> static void __iomem *wd_reset_base_addr;
> @@ -150,6 +151,7 @@ static int simatic_ipc_wdt_probe(struct
> platform_device *pdev) struct simatic_ipc_platform *plat =
> pdev->dev.platform_data; struct device *dev = &pdev->dev;
> struct resource *res;
> + int ret;
>
> switch (plat->devmode) {
> case SIMATIC_IPC_DEVICE_227E:
> @@ -190,15 +192,14 @@ static int simatic_ipc_wdt_probe(struct
> platform_device *pdev) if (plat->devmode == SIMATIC_IPC_DEVICE_427E) {
> res = &mem_resource;
>
> - /* get GPIO base from PCI */
> - res->start =
> simatic_ipc_get_membase0(PCI_DEVFN(0x1f, 1));
> - if (res->start == 0)
> - return -ENODEV;
> + ret = p2sb_bar(NULL, 0, res);
> + if (ret)
> + return ret;
>
> /* do the final address calculation */
> res->start = res->start + (GPIO_COMMUNITY0_PORT_ID
> << 16) + PAD_CFG_DW0_GPP_A_23;
> - res->end += res->start;
> + res->end = res->start + SZ_4 - 1;
>
> wd_reset_base_addr = devm_ioremap_resource(dev, res);
> if (IS_ERR(wd_reset_base_addr))
> diff --git a/include/linux/platform_data/x86/simatic-ipc-base.h
> b/include/linux/platform_data/x86/simatic-ipc-base.h index
> 62d2bc774067..39fefd48cf4d 100644 ---
> a/include/linux/platform_data/x86/simatic-ipc-base.h +++
> b/include/linux/platform_data/x86/simatic-ipc-base.h @@ -24,6 +24,4
> @@ struct simatic_ipc_platform { u8 devmode;
> };
>
> -u32 simatic_ipc_get_membase0(unsigned int p2sb);
> -
> #endif /* __PLATFORM_DATA_X86_SIMATIC_IPC_BASE_H */
Am Mon, 31 Jan 2022 17:13:38 +0200
schrieb Andy Shevchenko <[email protected]>:
> There are a few users and at least one more is coming (*) that would
> like to utilize P2SB mechanism of hiding and unhiding a device from
> the PCI configuration space.
>
> Here is the series to consolidate p2sb handling code for existing
> users and provide a generic way for new comer(s).
>
> It also includes a patch to enable GPIO controllers on Apollo Lake
> when it's used with ABL bootloader w/o ACPI support.
>
> The patch that bring the helper ("platform/x86/intel: Add Primary
> to Sideband (P2SB) bridge support") has a commit message that
> sheds a light on what the P2SB is and why this is needed.
>
> The changes made in v2 do not change the main idea and the
> functionality in a big scale. What we need is probably one more
> (RE-)test done by Henning. I hope to have it merged to v5.18-rc1 that
> Siemens can develop their changes based on this series.
I did test this series and it works as expected. Only problem is that
the leds driver will not work together with the pinctrl. Because two
"in tree drivers" will try to reserve the same memory region when both
are enabled. Who wins is a matter of probing order ...
If you can take my changes into your series we will not have a problem.
Otherwise we might need to create sort of a conflict which my series
would revert when switching apl lake to gpio.
I would not know the process, let us see what the reviews bring and how
to continue here.
Thanks so much for taking care, especially the pinctrl coming up
without ACPI really improves the simatic leds on the apl lake.
In fact i will have to double check if i really need the p2sb for the
427E wdt ... but until i have an answer, p2sb works just fine.
regards,
Henning
> I have tested this on Apollo Lake platform (I'm able to see SPI NOR
> and since we have an ACPI device for GPIO I do not see any attempts
> to recreate one).
>
> *) One in this series, and one is a due after merge in the Simatic
> IPC drivers
>
> The series may be routed either via MFD (and I guess Lee would prefer
> that) or via PDx86, whichever seems better for you, folks. As of
> today patches are ACKed by the respective maintainers, but I2C one
> and one of the MFD.
>
> Wolfram, can you ACK the patch against i2c-i801 driver, if you have no
> objections?
>
> Changes in v4:
> - added tag to the entire series (Hans)
> - added tag to pin control patch (Mika)
> - dropped PCI core changes (PCI core doesn't want modifications to be
> made)
> - as a consequence of the above merged necessary bits into p2sb.c
> - added a check that p2sb is really hidden (Hans)
> - added EDAC patches (reviewed by maintainer internally)
>
> Changes in v3:
> - resent with cover letter
>
> Changes in v2:
> - added parentheses around bus in macros (Joe)
> - added tag (Jean)
> - fixed indentation and wrapping in the header (Christoph)
> - moved out of PCI realm to PDx86 as the best common denominator
> (Bjorn)
> - added a verbose commit message to explain P2SB thingy (Bjorn)
> - converted first parameter from pci_dev to pci_bus
> - made first two parameters (bus and devfn) optional (Henning, Lee)
> - added Intel pin control patch to the series (Henning, Mika)
> - fixed English style in the commit message of one of MFD patch (Lee)
> - added tags to my MFD LPC ICH patches (Lee)
> - used consistently (c) (Lee)
> - made indexing for MFD cell and resource arrays (Lee)
> - fixed the resource size in i801 (Jean)
>
> Andy Shevchenko (6):
> pinctrl: intel: Check against matching data instead of ACPI
> companion mfd: lpc_ich: Factor out lpc_ich_enable_spi_write()
> mfd: lpc_ich: Switch to generic p2sb_bar()
> i2c: i801: convert to use common P2SB accessor
> EDAC, pnd2: Use proper I/O accessors and address space annotation
> EDAC, pnd2: convert to use common P2SB accessor
>
> Jonathan Yong (1):
> platform/x86/intel: Add Primary to Sideband (P2SB) bridge support
>
> Tan Jui Nee (1):
> mfd: lpc_ich: Add support for pinctrl in non-ACPI system
>
> drivers/edac/Kconfig | 1 +
> drivers/edac/pnd2_edac.c | 62 ++---
> drivers/i2c/busses/Kconfig | 1 +
> drivers/i2c/busses/i2c-i801.c | 39 +---
> drivers/mfd/Kconfig | 1 +
> drivers/mfd/lpc_ich.c | 136 +++++++++--
> drivers/pinctrl/intel/pinctrl-intel.c | 14 +-
> drivers/platform/x86/intel/Kconfig | 12 +
> drivers/platform/x86/intel/Makefile | 1 +
> drivers/platform/x86/intel/p2sb.c | 305
> +++++++++++++++++++++++++ include/linux/platform_data/x86/p2sb.h |
> 27 +++ 11 files changed, 500 insertions(+), 99 deletions(-)
> create mode 100644 drivers/platform/x86/intel/p2sb.c
> create mode 100644 include/linux/platform_data/x86/p2sb.h
>
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:
...
> > > > +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.
Right.
> In which case that comment is also confusing!
I will fix a comment in the next version.
--
With Best Regards,
Andy Shevchenko
On Tue, Mar 08, 2022 at 08:50:16PM +0100, Henning Schild wrote:
> Am Mon, 31 Jan 2022 17:13:38 +0200
> schrieb Andy Shevchenko <[email protected]>:
>
> > There are a few users and at least one more is coming (*) that would
> > like to utilize P2SB mechanism of hiding and unhiding a device from
> > the PCI configuration space.
> >
> > Here is the series to consolidate p2sb handling code for existing
> > users and provide a generic way for new comer(s).
> >
> > It also includes a patch to enable GPIO controllers on Apollo Lake
> > when it's used with ABL bootloader w/o ACPI support.
> >
> > The patch that bring the helper ("platform/x86/intel: Add Primary
> > to Sideband (P2SB) bridge support") has a commit message that
> > sheds a light on what the P2SB is and why this is needed.
> >
> > The changes made in v2 do not change the main idea and the
> > functionality in a big scale. What we need is probably one more
> > (RE-)test done by Henning. I hope to have it merged to v5.18-rc1 that
> > Siemens can develop their changes based on this series.
>
> I did test this series and it works as expected. Only problem is that
> the leds driver will not work together with the pinctrl. Because two
> "in tree drivers" will try to reserve the same memory region when both
> are enabled. Who wins is a matter of probing order ...
Can we have your formal Tested-by tag?
> If you can take my changes into your series we will not have a problem.
Yes, that's the plan, but your patches needs a bit of work I believe.
> Otherwise we might need to create sort of a conflict which my series
> would revert when switching apl lake to gpio.
>
> I would not know the process, let us see what the reviews bring and how
> to continue here.
I'm about to comment on the patches.
> Thanks so much for taking care, especially the pinctrl coming up
> without ACPI really improves the simatic leds on the apl lake.
You are welcome!
> In fact i will have to double check if i really need the p2sb for the
> 427E wdt ... but until i have an answer, p2sb works just fine.
Thanks!
> > I have tested this on Apollo Lake platform (I'm able to see SPI NOR
> > and since we have an ACPI device for GPIO I do not see any attempts
> > to recreate one).
> >
> > *) One in this series, and one is a due after merge in the Simatic
> > IPC drivers
> >
> > The series may be routed either via MFD (and I guess Lee would prefer
> > that) or via PDx86, whichever seems better for you, folks. As of
> > today patches are ACKed by the respective maintainers, but I2C one
> > and one of the MFD.
> >
> > Wolfram, can you ACK the patch against i2c-i801 driver, if you have no
> > objections?
> >
> > Changes in v4:
> > - added tag to the entire series (Hans)
> > - added tag to pin control patch (Mika)
> > - dropped PCI core changes (PCI core doesn't want modifications to be
> > made)
> > - as a consequence of the above merged necessary bits into p2sb.c
> > - added a check that p2sb is really hidden (Hans)
> > - added EDAC patches (reviewed by maintainer internally)
> >
> > Changes in v3:
> > - resent with cover letter
> >
> > Changes in v2:
> > - added parentheses around bus in macros (Joe)
> > - added tag (Jean)
> > - fixed indentation and wrapping in the header (Christoph)
> > - moved out of PCI realm to PDx86 as the best common denominator
> > (Bjorn)
> > - added a verbose commit message to explain P2SB thingy (Bjorn)
> > - converted first parameter from pci_dev to pci_bus
> > - made first two parameters (bus and devfn) optional (Henning, Lee)
> > - added Intel pin control patch to the series (Henning, Mika)
> > - fixed English style in the commit message of one of MFD patch (Lee)
> > - added tags to my MFD LPC ICH patches (Lee)
> > - used consistently (c) (Lee)
> > - made indexing for MFD cell and resource arrays (Lee)
> > - fixed the resource size in i801 (Jean)
> >
> > Andy Shevchenko (6):
> > pinctrl: intel: Check against matching data instead of ACPI
> > companion mfd: lpc_ich: Factor out lpc_ich_enable_spi_write()
> > mfd: lpc_ich: Switch to generic p2sb_bar()
> > i2c: i801: convert to use common P2SB accessor
> > EDAC, pnd2: Use proper I/O accessors and address space annotation
> > EDAC, pnd2: convert to use common P2SB accessor
> >
> > Jonathan Yong (1):
> > platform/x86/intel: Add Primary to Sideband (P2SB) bridge support
> >
> > Tan Jui Nee (1):
> > mfd: lpc_ich: Add support for pinctrl in non-ACPI system
> >
> > drivers/edac/Kconfig | 1 +
> > drivers/edac/pnd2_edac.c | 62 ++---
> > drivers/i2c/busses/Kconfig | 1 +
> > drivers/i2c/busses/i2c-i801.c | 39 +---
> > drivers/mfd/Kconfig | 1 +
> > drivers/mfd/lpc_ich.c | 136 +++++++++--
> > drivers/pinctrl/intel/pinctrl-intel.c | 14 +-
> > drivers/platform/x86/intel/Kconfig | 12 +
> > drivers/platform/x86/intel/Makefile | 1 +
> > drivers/platform/x86/intel/p2sb.c | 305
> > +++++++++++++++++++++++++ include/linux/platform_data/x86/p2sb.h |
> > 27 +++ 11 files changed, 500 insertions(+), 99 deletions(-)
> > create mode 100644 drivers/platform/x86/intel/p2sb.c
> > create mode 100644 include/linux/platform_data/x86/p2sb.h
> >
>
--
With Best Regards,
Andy Shevchenko
Am Wed, 4 May 2022 15:51:01 +0300
schrieb Andy Shevchenko <[email protected]>:
> On Tue, Mar 08, 2022 at 08:35:20PM +0100, Henning Schild wrote:
> > This switches the simatic-ipc modules to using the p2sb interface
> > introduced by Andy with "platform/x86: introduce p2sb_bar() helper".
> >
> > It also switches to one apollo lake device to using gpio leds.
> >
> > I am kind of hoping Andy will take this on top and propose it in his
> > series.
>
> First of all, they are not applicable to my current version [1] of
> the series (it maybe something changed in the Simatic drivers
> upstream, because I have got conflicts there. For the record, I'm
> using Linux Next as a base.
That is possible, some sparse findings have been fixed lately.
> Second question is could it be possible to split first patch into
> three, or it has to be in one?
I assume one for leds one for wdt and finally drop stuff from platform,
and i will go with that assumption for a next round based on your tree
directly.
Can you explain why that will be useful? While it is kind of a
separation of concerns and subsystems ... it also kind of all belongs
together and needs to be merged in a rather strict order.
regards,
Henning
> [1]: https://gitlab.com/andy-shev/next/-/tree/topic/p2sb-next
> It would be nice if you can perform another round of testing.
>
> > Henning Schild (2):
> > simatic-ipc: convert to use common P2SB accessor
> > leds: simatic-ipc-leds-gpio: add GPIO version of Siemens driver
> >
> > drivers/leds/simple/Kconfig | 11 ++
> > drivers/leds/simple/Makefile | 3 +-
> > drivers/leds/simple/simatic-ipc-leds-gpio.c | 108
> > ++++++++++++++++++ drivers/leds/simple/simatic-ipc-leds.c |
> > 77 +------------ drivers/platform/x86/simatic-ipc.c |
> > 43 +------ drivers/watchdog/Kconfig | 1 +
> > drivers/watchdog/simatic-ipc-wdt.c | 15 +--
> > .../platform_data/x86/simatic-ipc-base.h | 2 -
> > 8 files changed, 139 insertions(+), 121 deletions(-)
> > create mode 100644 drivers/leds/simple/simatic-ipc-leds-gpio.c
> >
> > --
> > 2.34.1
> >
>
On Tue, Mar 08, 2022 at 08:35:20PM +0100, Henning Schild wrote:
> This switches the simatic-ipc modules to using the p2sb interface
> introduced by Andy with "platform/x86: introduce p2sb_bar() helper".
>
> It also switches to one apollo lake device to using gpio leds.
>
> I am kind of hoping Andy will take this on top and propose it in his
> series.
First of all, they are not applicable to my current version [1] of the series
(it maybe something changed in the Simatic drivers upstream, because I have got
conflicts there. For the record, I'm using Linux Next as a base.
Second question is could it be possible to split first patch into three, or it
has to be in one?
[1]: https://gitlab.com/andy-shev/next/-/tree/topic/p2sb-next
It would be nice if you can perform another round of testing.
> Henning Schild (2):
> simatic-ipc: convert to use common P2SB accessor
> leds: simatic-ipc-leds-gpio: add GPIO version of Siemens driver
>
> drivers/leds/simple/Kconfig | 11 ++
> drivers/leds/simple/Makefile | 3 +-
> drivers/leds/simple/simatic-ipc-leds-gpio.c | 108 ++++++++++++++++++
> drivers/leds/simple/simatic-ipc-leds.c | 77 +------------
> drivers/platform/x86/simatic-ipc.c | 43 +------
> drivers/watchdog/Kconfig | 1 +
> drivers/watchdog/simatic-ipc-wdt.c | 15 +--
> .../platform_data/x86/simatic-ipc-base.h | 2 -
> 8 files changed, 139 insertions(+), 121 deletions(-)
> create mode 100644 drivers/leds/simple/simatic-ipc-leds-gpio.c
>
> --
> 2.34.1
>
--
With Best Regards,
Andy Shevchenko
On Wed, May 4, 2022 at 6:16 PM Henning Schild
<[email protected]> wrote:
> Am Wed, 4 May 2022 15:51:01 +0300
> schrieb Andy Shevchenko <[email protected]>:
> > On Tue, Mar 08, 2022 at 08:35:20PM +0100, Henning Schild wrote:
...
> > Second question is could it be possible to split first patch into
> > three, or it has to be in one?
>
> I assume one for leds one for wdt and finally drop stuff from platform,
Yes.
> and i will go with that assumption for a next round based on your tree
> directly.
> Can you explain why that will be useful? While it is kind of a
> separation of concerns and subsystems ... it also kind of all belongs
> together and needs to be merged in a rather strict order.
The main case here is that it's easy to review during upstreaming and
in case of somebody looking into the history. It keeps each of the
changes logically isolated. I.o.w. it adds flexibility, for example
changing ordering of the WDT and LED patches in the series in this
case.
I admit that for _this_ series my arguments are not strong, but I'm
speaking out of general approach. The pattern
1) add new api
2) switch driver #1 to it
...
2+n) switch driver #n to it
3+n) drop old API
is how we do in the Linux kernel, even if the changes are coupled
together from a functional / compile perspective.
--
With Best Regards,
Andy Shevchenko
Am Wed, 4 May 2022 15:42:29 +0300
schrieb Andy Shevchenko <[email protected]>:
> On Tue, Mar 08, 2022 at 08:50:16PM +0100, Henning Schild wrote:
> > Am Mon, 31 Jan 2022 17:13:38 +0200
> > schrieb Andy Shevchenko <[email protected]>:
> >
> > > There are a few users and at least one more is coming (*) that
> > > would like to utilize P2SB mechanism of hiding and unhiding a
> > > device from the PCI configuration space.
> > >
> > > Here is the series to consolidate p2sb handling code for existing
> > > users and provide a generic way for new comer(s).
> > >
> > > It also includes a patch to enable GPIO controllers on Apollo Lake
> > > when it's used with ABL bootloader w/o ACPI support.
> > >
> > > The patch that bring the helper ("platform/x86/intel: Add Primary
> > > to Sideband (P2SB) bridge support") has a commit message that
> > > sheds a light on what the P2SB is and why this is needed.
> > >
> > > The changes made in v2 do not change the main idea and the
> > > functionality in a big scale. What we need is probably one more
> > > (RE-)test done by Henning. I hope to have it merged to v5.18-rc1
> > > that Siemens can develop their changes based on this series.
> >
> > I did test this series and it works as expected. Only problem is
> > that the leds driver will not work together with the pinctrl.
> > Because two "in tree drivers" will try to reserve the same memory
> > region when both are enabled. Who wins is a matter of probing order
> > ...
>
> Can we have your formal Tested-by tag?
Sure. I will just put it here for you to take.
Tested-by: Henning Schild <[email protected]>
Let me know if you need it in another shape or form.
Henning
>
> > If you can take my changes into your series we will not have a
> > problem.
>
> Yes, that's the plan, but your patches needs a bit of work I believe.
>
> > Otherwise we might need to create sort of a conflict which my series
> > would revert when switching apl lake to gpio.
> >
> > I would not know the process, let us see what the reviews bring and
> > how to continue here.
>
> I'm about to comment on the patches.
>
> > Thanks so much for taking care, especially the pinctrl coming up
> > without ACPI really improves the simatic leds on the apl lake.
>
> You are welcome!
>
> > In fact i will have to double check if i really need the p2sb for
> > the 427E wdt ... but until i have an answer, p2sb works just fine.
>
> Thanks!
>
> > > I have tested this on Apollo Lake platform (I'm able to see SPI
> > > NOR and since we have an ACPI device for GPIO I do not see any
> > > attempts to recreate one).
> > >
> > > *) One in this series, and one is a due after merge in the Simatic
> > > IPC drivers
> > >
> > > The series may be routed either via MFD (and I guess Lee would
> > > prefer that) or via PDx86, whichever seems better for you, folks.
> > > As of today patches are ACKed by the respective maintainers, but
> > > I2C one and one of the MFD.
> > >
> > > Wolfram, can you ACK the patch against i2c-i801 driver, if you
> > > have no objections?
> > >
> > > Changes in v4:
> > > - added tag to the entire series (Hans)
> > > - added tag to pin control patch (Mika)
> > > - dropped PCI core changes (PCI core doesn't want modifications
> > > to be made)
> > > - as a consequence of the above merged necessary bits into p2sb.c
> > > - added a check that p2sb is really hidden (Hans)
> > > - added EDAC patches (reviewed by maintainer internally)
> > >
> > > Changes in v3:
> > > - resent with cover letter
> > >
> > > Changes in v2:
> > > - added parentheses around bus in macros (Joe)
> > > - added tag (Jean)
> > > - fixed indentation and wrapping in the header (Christoph)
> > > - moved out of PCI realm to PDx86 as the best common denominator
> > > (Bjorn)
> > > - added a verbose commit message to explain P2SB thingy (Bjorn)
> > > - converted first parameter from pci_dev to pci_bus
> > > - made first two parameters (bus and devfn) optional (Henning,
> > > Lee)
> > > - added Intel pin control patch to the series (Henning, Mika)
> > > - fixed English style in the commit message of one of MFD patch
> > > (Lee)
> > > - added tags to my MFD LPC ICH patches (Lee)
> > > - used consistently (c) (Lee)
> > > - made indexing for MFD cell and resource arrays (Lee)
> > > - fixed the resource size in i801 (Jean)
> > >
> > > Andy Shevchenko (6):
> > > pinctrl: intel: Check against matching data instead of ACPI
> > > companion mfd: lpc_ich: Factor out lpc_ich_enable_spi_write()
> > > mfd: lpc_ich: Switch to generic p2sb_bar()
> > > i2c: i801: convert to use common P2SB accessor
> > > EDAC, pnd2: Use proper I/O accessors and address space
> > > annotation EDAC, pnd2: convert to use common P2SB accessor
> > >
> > > Jonathan Yong (1):
> > > platform/x86/intel: Add Primary to Sideband (P2SB) bridge
> > > support
> > >
> > > Tan Jui Nee (1):
> > > mfd: lpc_ich: Add support for pinctrl in non-ACPI system
> > >
> > > drivers/edac/Kconfig | 1 +
> > > drivers/edac/pnd2_edac.c | 62 ++---
> > > drivers/i2c/busses/Kconfig | 1 +
> > > drivers/i2c/busses/i2c-i801.c | 39 +---
> > > drivers/mfd/Kconfig | 1 +
> > > drivers/mfd/lpc_ich.c | 136 +++++++++--
> > > drivers/pinctrl/intel/pinctrl-intel.c | 14 +-
> > > drivers/platform/x86/intel/Kconfig | 12 +
> > > drivers/platform/x86/intel/Makefile | 1 +
> > > drivers/platform/x86/intel/p2sb.c | 305
> > > +++++++++++++++++++++++++ include/linux/platform_data/x86/p2sb.h |
> > > 27 +++ 11 files changed, 500 insertions(+), 99 deletions(-)
> > > create mode 100644 drivers/platform/x86/intel/p2sb.c
> > > create mode 100644 include/linux/platform_data/x86/p2sb.h
> > >
> >
>
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.
--
With Best Regards,
Andy Shevchenko
On Wed, May 04, 2022 at 05:55:26PM +0200, Andy Shevchenko wrote:
> On Wed, May 4, 2022 at 5:10 PM Henning Schild
> <[email protected]> wrote:
> > Am Wed, 4 May 2022 15:42:29 +0300
> > schrieb Andy Shevchenko <[email protected]>:
> ...
> > Let me know if you need it in another shape or form.
>
> Form is okay, just would be nice if you can retest what I have in the
> branch as an updated version.
Is there any news? I would like to send a new version sooner than later.
We may also apply the patches from this series and when you will be ready
apply yours.
--
With Best Regards,
Andy Shevchenko
On Wed, May 04, 2022 at 05:19:51PM +0200, Henning Schild wrote:
> Am Wed, 4 May 2022 15:51:01 +0300
> schrieb Andy Shevchenko <[email protected]>:
>
> > On Tue, Mar 08, 2022 at 08:35:20PM +0100, Henning Schild wrote:
> > > This switches the simatic-ipc modules to using the p2sb interface
> > > introduced by Andy with "platform/x86: introduce p2sb_bar() helper".
> > >
> > > It also switches to one apollo lake device to using gpio leds.
> > >
> > > I am kind of hoping Andy will take this on top and propose it in his
> > > series.
> >
> > First of all, they are not applicable to my current version [1] of
> > the series (it maybe something changed in the Simatic drivers
> > upstream, because I have got conflicts there. For the record, I'm
> > using Linux Next as a base.
>
> That is possible, some sparse findings have been fixed lately.
>
> > Second question is could it be possible to split first patch into
> > three, or it has to be in one?
>
> I assume one for leds one for wdt and finally drop stuff from platform,
> and i will go with that assumption for a next round based on your tree
> directly.
> Can you explain why that will be useful? While it is kind of a
> separation of concerns and subsystems ... it also kind of all belongs
> together and needs to be merged in a rather strict order.
>
That is not really correct. It should be possible to split
the patches and only remove simatic_ipc_get_membase0() after the
other patches have been applied.
On a side note, neither subject nor description of patch 1/2
mention that the patch touches both LED and watchdog code, which
is at the very least bad style.
Guenter
> regards,
> Henning
>
> > [1]: https://gitlab.com/andy-shev/next/-/tree/topic/p2sb-next
> > It would be nice if you can perform another round of testing.
> >
> > > Henning Schild (2):
> > > simatic-ipc: convert to use common P2SB accessor
> > > leds: simatic-ipc-leds-gpio: add GPIO version of Siemens driver
> > >
> > > drivers/leds/simple/Kconfig | 11 ++
> > > drivers/leds/simple/Makefile | 3 +-
> > > drivers/leds/simple/simatic-ipc-leds-gpio.c | 108
> > > ++++++++++++++++++ drivers/leds/simple/simatic-ipc-leds.c |
> > > 77 +------------ drivers/platform/x86/simatic-ipc.c |
> > > 43 +------ drivers/watchdog/Kconfig | 1 +
> > > drivers/watchdog/simatic-ipc-wdt.c | 15 +--
> > > .../platform_data/x86/simatic-ipc-base.h | 2 -
> > > 8 files changed, 139 insertions(+), 121 deletions(-)
> > > create mode 100644 drivers/leds/simple/simatic-ipc-leds-gpio.c
> > >
> > > --
> > > 2.34.1
> > >
> >
>
On Wed, May 4, 2022 at 5:10 PM Henning Schild
<[email protected]> wrote:
> Am Wed, 4 May 2022 15:42:29 +0300
> schrieb Andy Shevchenko <[email protected]>:
> > On Tue, Mar 08, 2022 at 08:50:16PM +0100, Henning Schild wrote:
...
> > Can we have your formal Tested-by tag?
>
> Sure. I will just put it here for you to take.
>
> Tested-by: Henning Schild <[email protected]>
Thank you!
> Let me know if you need it in another shape or form.
Form is okay, just would be nice if you can retest what I have in the
branch as an updated version.
--
With Best Regards,
Andy Shevchenko
On Tue, May 10, 2022 at 05:30:53PM +0200, Henning Schild wrote:
> Am Wed, 4 May 2022 17:19:51 +0200
> schrieb Henning Schild <[email protected]>:
> > Am Wed, 4 May 2022 15:51:01 +0300
> > schrieb Andy Shevchenko <[email protected]>:
> > > On Tue, Mar 08, 2022 at 08:35:20PM +0100, Henning Schild wrote:
...
> > > [1]: https://gitlab.com/andy-shev/next/-/tree/topic/p2sb-next
> > > It would be nice if you can perform another round of testing.
>
> Just got around to testing this with my patches on top. My stuff will
> need some more work before i can send again.
>
> Is this a rebasing branch?
> With efc7d77ea372 ("EDAC, pnd2: convert to use common P2SB accessor")
It's rebased over and over. I just pushed the same version I have sent as v5.
> I am seeing problems while booting ... things do work but still error
> messages which probably should not be there.
It's okay. This is not related to my stuff, it's a new series from Marc which
enables that (harmless) warning.
> [ 2.217506] broxton-pinctrl apollolake-pinctrl.1: Failed to attach ACPI GPIO chip
> [ 2.217542] gpio gpiochip1: (apollolake-pinctrl.1): not an immutable chip, please consider fixing it!
> [ 2.217771] i801_smbus 0000:00:1f.1: Failed to enable SMBus PCI device (-22)
> [ 2.217788] i801_smbus: probe of 0000:00:1f.1 failed with error -22
> [ 2.221460] broxton-pinctrl apollolake-pinctrl.2: Failed to attach ACPI GPIO chip
> [ 2.221482] gpio gpiochip2: (apollolake-pinctrl.2): not an immutable chip, please consider fixing it!
> [ 2.222010] broxton-pinctrl apollolake-pinctrl.3: Failed to attach ACPI GPIO chip
> [ 2.222023] gpio gpiochip3: (apollolake-pinctrl.3): not an immutable
> chip, please consider fixing it!
--
With Best Regards,
Andy Shevchenko
Am Wed, 4 May 2022 17:19:51 +0200
schrieb Henning Schild <[email protected]>:
> Am Wed, 4 May 2022 15:51:01 +0300
> schrieb Andy Shevchenko <[email protected]>:
>
> > On Tue, Mar 08, 2022 at 08:35:20PM +0100, Henning Schild wrote:
> > > This switches the simatic-ipc modules to using the p2sb interface
> > > introduced by Andy with "platform/x86: introduce p2sb_bar()
> > > helper".
> > >
> > > It also switches to one apollo lake device to using gpio leds.
> > >
> > > I am kind of hoping Andy will take this on top and propose it in
> > > his series.
> >
> > First of all, they are not applicable to my current version [1] of
> > the series (it maybe something changed in the Simatic drivers
> > upstream, because I have got conflicts there. For the record, I'm
> > using Linux Next as a base.
>
> That is possible, some sparse findings have been fixed lately.
>
> > Second question is could it be possible to split first patch into
> > three, or it has to be in one?
>
> I assume one for leds one for wdt and finally drop stuff from
> platform, and i will go with that assumption for a next round based
> on your tree directly.
> Can you explain why that will be useful? While it is kind of a
> separation of concerns and subsystems ... it also kind of all belongs
> together and needs to be merged in a rather strict order.
>
> regards,
> Henning
>
> > [1]: https://gitlab.com/andy-shev/next/-/tree/topic/p2sb-next
> > It would be nice if you can perform another round of testing.
Just got around to testing this with my patches on top. My stuff will
need some more work before i can send again.
Is this a rebasing branch?
With efc7d77ea372 ("EDAC, pnd2: convert to use common P2SB accessor")
I am seeing problems while booting ... things do work but still error
messages which probably should not be there.
[ 2.215715] gpio gpiochip0: (apollolake-pinctrl.0): not an immutable chip, please consider fixing it!
[ 2.217506] broxton-pinctrl apollolake-pinctrl.1: Failed to attach ACPI GPIO chip
[ 2.217542] gpio gpiochip1: (apollolake-pinctrl.1): not an immutable chip, please consider fixing it!
[ 2.217771] i801_smbus 0000:00:1f.1: Failed to enable SMBus PCI device (-22)
[ 2.217788] i801_smbus: probe of 0000:00:1f.1 failed with error -22
[ 2.221460] broxton-pinctrl apollolake-pinctrl.2: Failed to attach ACPI GPIO chip
[ 2.221482] gpio gpiochip2: (apollolake-pinctrl.2): not an immutable chip, please consider fixing it!
[ 2.222010] broxton-pinctrl apollolake-pinctrl.3: Failed to attach ACPI GPIO chip
[ 2.222023] gpio gpiochip3: (apollolake-pinctrl.3): not an immutable
chip, please consider fixing it!
regards,
Henning
> > > Henning Schild (2):
> > > simatic-ipc: convert to use common P2SB accessor
> > > leds: simatic-ipc-leds-gpio: add GPIO version of Siemens driver
> > >
> > > drivers/leds/simple/Kconfig | 11 ++
> > > drivers/leds/simple/Makefile | 3 +-
> > > drivers/leds/simple/simatic-ipc-leds-gpio.c | 108
> > > ++++++++++++++++++ drivers/leds/simple/simatic-ipc-leds.c |
> > > 77 +------------ drivers/platform/x86/simatic-ipc.c |
> > > 43 +------ drivers/watchdog/Kconfig | 1 +
> > > drivers/watchdog/simatic-ipc-wdt.c | 15 +--
> > > .../platform_data/x86/simatic-ipc-base.h | 2 -
> > > 8 files changed, 139 insertions(+), 121 deletions(-)
> > > create mode 100644 drivers/leds/simple/simatic-ipc-leds-gpio.c
> > >
> > > --
> > > 2.34.1
> > >
> >
>