Hi!

> > > Can we just wait for Pavel's review, then merge them all at once?
> >
> > 10,12: Acked-by: Pavel Machek <[email protected]>
>
> Thanks Pavel. I'll get that added.

Thank you, sorry for the delays.

Best regards,
Pavel

--
People of Russia, stop Putin before his war on Ukraine escalates.

Attachments:

(No filename) (320.00 B)
signature.asc (201.00 B)
Download all attachments

2022-07-18 11:53:27

by Andy Shevchenko

[permalink] [raw]

Subject: Re: [PATCH v6 00/12] platform/x86: introduce p2sb_bar() helper

On Mon, Jun 06, 2022 at 07:41:26PM +0300, Andy Shevchenko wrote:
> There are a few users 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 to 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 brings 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.
>
> 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).
>
> The series is ready to be merged via MFD tree, but see below.
>
> The series also includes updates for Simatic IPC drivers that partially
> tagged by respective maintainers (the main question is if Pavel is okay
> with the last three patches, since I believe Hans is okay with removing
> some code under PDx86). Hence the first 8 patches can be merged right
> away and the rest when Pavel does his review.

Kernel test bot seems found an issue with dependencies, because selection
of P2SB is not enough.

There are two solutions that I can see now:
1) move P2SB out of X86_PLATFORM_DEVICES section (like PMC_ATOM);
2) add 'depends on X86_PLATFORM_DEVICES' to the affected drivers.

I think the first solution cleaner, because it would be strange to have
the dependency on the drivers that quite unlikely be on server platforms
(e.g. EDAC).

In long term perhaps something like drivers/platform/x86/lib which is for
platform libraries or so and independent on X86_PLATFORM_DEVICES?

I will send a fix soon as per 1) above, feel free to comment here or there.

--
With Best Regards,
Andy Shevchenko

2022-08-01 14:28:26

by Henning Schild

[permalink] [raw]

Subject: Re: [PATCH v6 12/12] leds: simatic-ipc-leds-gpio: add GPIO version of Siemens driver

Am Mon, 6 Jun 2022 19:41:38 +0300
schrieb Andy Shevchenko <[email protected]>:

> From: Henning Schild <[email protected]>
>
> On Apollo Lake the pinctrl drivers will now come up without ACPI. Use
> that instead of open coding it.
> Create a new driver for that which can later be filled with more GPIO
> based models, and which has different dependencies.
>
> Signed-off-by: Henning Schild <[email protected]>
> Signed-off-by: Andy Shevchenko <[email protected]>
> ---
> drivers/leds/simple/Kconfig | 7 +-
> drivers/leds/simple/Makefile | 1 +
> drivers/leds/simple/simatic-ipc-leds-gpio.c | 105
> ++++++++++++++++++++ drivers/leds/simple/simatic-ipc-leds.c |
> 80 +-------------- drivers/platform/x86/simatic-ipc.c | 5
> +- 5 files changed, 117 insertions(+), 81 deletions(-)
> create mode 100644 drivers/leds/simple/simatic-ipc-leds-gpio.c
>
> diff --git a/drivers/leds/simple/Kconfig b/drivers/leds/simple/Kconfig
> index bbf8cff3c3f6..fd2b8225d926 100644
> --- a/drivers/leds/simple/Kconfig
> +++ b/drivers/leds/simple/Kconfig
> @@ -1,12 +1,11 @@
> # SPDX-License-Identifier: GPL-2.0-only
> config LEDS_SIEMENS_SIMATIC_IPC
> tristate "LED driver for Siemens Simatic IPCs"
> - depends on LEDS_CLASS
> + depends on LEDS_GPIO
> depends on SIEMENS_SIMATIC_IPC
> - select P2SB
> help
> This option enables support for the LEDs of several
> Industrial PCs from Siemens.
>
> - To compile this driver as a module, choose M here: the
> module
> - will be called simatic-ipc-leds.
> + To compile this driver as a module, choose M here: the
> modules
> + will be called simatic-ipc-leds and simatic-ipc-leds-gpio.
> diff --git a/drivers/leds/simple/Makefile
> b/drivers/leds/simple/Makefile index 8481f1e9e360..1c7ef5e1324b 100644
> --- a/drivers/leds/simple/Makefile
> +++ b/drivers/leds/simple/Makefile
> @@ -1,2 +1,3 @@
> # SPDX-License-Identifier: GPL-2.0
> obj-$(CONFIG_LEDS_SIEMENS_SIMATIC_IPC) += simatic-ipc-leds.o
> +obj-$(CONFIG_LEDS_SIEMENS_SIMATIC_IPC) +=
> simatic-ipc-leds-gpio.o diff --git
> a/drivers/leds/simple/simatic-ipc-leds-gpio.c
> b/drivers/leds/simple/simatic-ipc-leds-gpio.c new file mode 100644
> index 000000000000..4c9e663a90ba --- /dev/null
> +++ b/drivers/leds/simple/simatic-ipc-leds-gpio.c
> @@ -0,0 +1,105 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Siemens SIMATIC IPC driver for GPIO based LEDs
> + *
> + * Copyright (c) Siemens AG, 2022
> + *
> + * Authors:
> + * Henning Schild <[email protected]>
> + */
> +
> +#include <linux/gpio/machine.h>
> +#include <linux/gpio/consumer.h>
> +#include <linux/leds.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +
> +static struct gpiod_lookup_table simatic_ipc_led_gpio_table = {
> + .dev_id = "leds-gpio",
> + .table = {
> + GPIO_LOOKUP_IDX("apollolake-pinctrl.0", 51, NULL, 0,
> GPIO_ACTIVE_LOW),
> + GPIO_LOOKUP_IDX("apollolake-pinctrl.0", 52, NULL, 1,
> GPIO_ACTIVE_LOW),
> + GPIO_LOOKUP_IDX("apollolake-pinctrl.0", 53, NULL, 2,
> GPIO_ACTIVE_LOW),
> + GPIO_LOOKUP_IDX("apollolake-pinctrl.0", 57, NULL, 3,
> GPIO_ACTIVE_LOW),
> + GPIO_LOOKUP_IDX("apollolake-pinctrl.0", 58, NULL, 4,
> GPIO_ACTIVE_LOW),
> + GPIO_LOOKUP_IDX("apollolake-pinctrl.0", 60, NULL, 5,
> GPIO_ACTIVE_LOW),
> + GPIO_LOOKUP_IDX("apollolake-pinctrl.0", 56, NULL, 6,
> GPIO_ACTIVE_LOW),
> + GPIO_LOOKUP_IDX("apollolake-pinctrl.0", 59, NULL, 7,
> GPIO_ACTIVE_HIGH),
> + },
> +};
> +
> +static const struct gpio_led simatic_ipc_gpio_leds[] = {
> + { .name = "green:" LED_FUNCTION_STATUS "-3" },
> + { .name = "red:" LED_FUNCTION_STATUS "-1" },
> + { .name = "green:" LED_FUNCTION_STATUS "-1" },
> + { .name = "red:" LED_FUNCTION_STATUS "-2" },
> + { .name = "green:" LED_FUNCTION_STATUS "-2" },
> + { .name = "red:" LED_FUNCTION_STATUS "-3" },
> +};
> +
> +static const struct gpio_led_platform_data
> simatic_ipc_gpio_leds_pdata = {
> + .num_leds = ARRAY_SIZE(simatic_ipc_gpio_leds),
> + .leds = simatic_ipc_gpio_leds,
> +};
> +
> +static struct platform_device *simatic_leds_pdev;
> +
> +static int simatic_ipc_leds_gpio_remove(struct platform_device *pdev)
> +{
> + gpiod_remove_lookup_table(&simatic_ipc_led_gpio_table);
> + platform_device_unregister(simatic_leds_pdev);
> +
> + return 0;
> +}
> +
> +static int simatic_ipc_leds_gpio_probe(struct platform_device *pdev)
> +{
> + struct gpio_desc *gpiod;
> + int err;
> +
> + gpiod_add_lookup_table(&simatic_ipc_led_gpio_table);
> + simatic_leds_pdev = platform_device_register_resndata(NULL,
> + "leds-gpio", PLATFORM_DEVID_NONE, NULL, 0,
> + &simatic_ipc_gpio_leds_pdata,
> + sizeof(simatic_ipc_gpio_leds_pdata));

If those GPIOs can not be found that leads to a pretty severe
logging/polling endless loop.

...
[ 34.017017] leds-gpio leds-gpio: Skipping unavailable LED gpio 0 (red:status-2)
[ 34.017038] leds-gpio leds-gpio: cannot find GPIO chip apollolake-pinctrl.0, deferring
[ 34.017158] leds-gpio leds-gpio: Skipping unavailable LED gpio 0 (red:status-2)
[ 34.017161] leds-gpio leds-gpio: cannot find GPIO chip apollolake-pinctrl.0, deferring
[ 34.017163] leds-gpio leds-gpio: Skipping unavailable LED gpio 0 (green:status-2)
[ 34.017166] leds-gpio leds-gpio: cannot find GPIO chip apollolake-pinctrl.0, deferring
[ 34.017168] leds-gpio leds-gpio: Skipping unavailable LED gpio 0 (red:status-3)
[ 34.017179] leds-gpio leds-gpio: cannot find GPIO chip apollolake-pinctrl.0, deferring
[ 34.017275] leds-gpio leds-gpio: cannot find GPIO chip apollolake-pinctrl.0, deferring
[ 34.017279] leds-gpio leds-gpio: Skipping unavailable LED gpio 0 (green:status-3)
...

This can be seen when the kernel is build without
CONFIG_PINCTRL_BROXTON. Can anyone please give advise on how to proceed
here.

I could do something like i proposed in "[PATCH 3/4] leds:
simatic-ipc-leds-gpio: add new model 227G".

if (!IS_ENABLED(CONFIG_PINCTRL_BROXTON)) return -ENO...

before registering the platform device.

An alternative could be to try and handle that -EPROBE_DEFER but that
might be hard because it might well fail a couple of times before
eventually that gpio driver is up, so i think i will send an update
implementing that !IS_ENABLED.

Ideally the gpio subsystem or leds-gpio would break that
polling/printing loop for me, or make sure it would not happen in the
first place.

regards,
Henning

> + if (IS_ERR(simatic_leds_pdev)) {
> + err = PTR_ERR(simatic_leds_pdev);
> + goto out;
> + }
> +
> + /* PM_BIOS_BOOT_N */
> + gpiod = gpiod_get_index(&simatic_leds_pdev->dev, NULL, 6,
> GPIOD_OUT_LOW);
> + if (IS_ERR(gpiod)) {
> + err = PTR_ERR(gpiod);
> + goto out;
> + }
> + gpiod_put(gpiod);
> +
> + /* PM_WDT_OUT */
> + gpiod = gpiod_get_index(&simatic_leds_pdev->dev, NULL, 7,
> GPIOD_OUT_LOW);
> + if (IS_ERR(gpiod)) {
> + err = PTR_ERR(gpiod);
> + goto out;
> + }
> + gpiod_put(gpiod);
> +
> + return 0;
> +out:
> + simatic_ipc_leds_gpio_remove(pdev);
> +
> + return err;
> +}
> +
> +static struct platform_driver simatic_ipc_led_gpio_driver = {
> + .probe = simatic_ipc_leds_gpio_probe,
> + .remove = simatic_ipc_leds_gpio_remove,
> + .driver = {
> + .name = KBUILD_MODNAME,
> + }
> +};
> +module_platform_driver(simatic_ipc_led_gpio_driver);
> +
> +MODULE_LICENSE("GPL v2");
> +MODULE_ALIAS("platform:" KBUILD_MODNAME);
> +MODULE_SOFTDEP("pre: platform:leds-gpio");
> +MODULE_AUTHOR("Henning Schild <[email protected]>");
> diff --git a/drivers/leds/simple/simatic-ipc-leds.c
> b/drivers/leds/simple/simatic-ipc-leds.c index
> 2e7597c143d8..4894c228c165 100644 ---
> a/drivers/leds/simple/simatic-ipc-leds.c +++
> b/drivers/leds/simple/simatic-ipc-leds.c @@ -15,7 +15,6 @@
> #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>
> @@ -24,7 +23,7 @@
> #define SIMATIC_IPC_LED_PORT_BASE 0x404E
>
> struct simatic_ipc_led {
> - unsigned int value; /* mask for io and offset for mem */
> + unsigned int value; /* mask for io */
> char *name;
> struct led_classdev cdev;
> };
> @@ -39,21 +38,6 @@ static struct simatic_ipc_led
> simatic_ipc_leds_io[] = { { }
> };
>
> -/* the actual start will be discovered with p2sb, 0 is a placeholder
> */ -static struct resource simatic_ipc_led_mem_res =
> DEFINE_RES_MEM_NAMED(0, 0, KBUILD_MODNAME); -
> -static void __iomem *simatic_ipc_led_memory;
> -
> -static struct simatic_ipc_led simatic_ipc_leds_mem[] = {
> - {0x500 + 0x1A0, "red:" LED_FUNCTION_STATUS "-1"},
> - {0x500 + 0x1A8, "green:" LED_FUNCTION_STATUS "-1"},
> - {0x500 + 0x1C8, "red:" LED_FUNCTION_STATUS "-2"},
> - {0x500 + 0x1D0, "green:" LED_FUNCTION_STATUS "-2"},
> - {0x500 + 0x1E0, "red:" LED_FUNCTION_STATUS "-3"},
> - {0x500 + 0x198, "green:" LED_FUNCTION_STATUS "-3"},
> - { }
> -};
> -
> static struct resource simatic_ipc_led_io_res =
> DEFINE_RES_IO_NAMED(SIMATIC_IPC_LED_PORT_BASE, SZ_2,
> KBUILD_MODNAME);
> @@ -89,28 +73,6 @@ static enum led_brightness
> simatic_ipc_led_get_io(struct led_classdev *led_cd) return
> inw(SIMATIC_IPC_LED_PORT_BASE) & led->value ? LED_OFF :
> led_cd->max_brightness; }
> -static void simatic_ipc_led_set_mem(struct led_classdev *led_cd,
> - enum led_brightness brightness)
> -{
> - struct simatic_ipc_led *led = cdev_to_led(led_cd);
> - void __iomem *reg = simatic_ipc_led_memory + led->value;
> - u32 val;
> -
> - val = readl(reg);
> - val = (val & ~1) | (brightness == LED_OFF);
> - writel(val, reg);
> -}
> -
> -static enum led_brightness simatic_ipc_led_get_mem(struct
> led_classdev *led_cd) -{
> - struct simatic_ipc_led *led = cdev_to_led(led_cd);
> - void __iomem *reg = simatic_ipc_led_memory + led->value;
> - u32 val;
> -
> - val = readl(reg);
> - return (val & 1) ? LED_OFF : led_cd->max_brightness;
> -}
> -
> static int simatic_ipc_leds_probe(struct platform_device *pdev)
> {
> const struct simatic_ipc_platform *plat =
> pdev->dev.platform_data; @@ -118,9 +80,7 @@ static int
> simatic_ipc_leds_probe(struct platform_device *pdev) struct
> simatic_ipc_led *ipcled; struct led_classdev *cdev;
> struct resource *res;
> - void __iomem *reg;
> - int err, type;
> - u32 val;
> + int err;
>
> switch (plat->devmode) {
> case SIMATIC_IPC_DEVICE_227D:
> @@ -135,51 +95,19 @@ static int simatic_ipc_leds_probe(struct
> platform_device *pdev) }
> ipcled = simatic_ipc_leds_io;
> }
> - type = IORESOURCE_IO;
> if (!devm_request_region(dev, res->start,
> resource_size(res), KBUILD_MODNAME)) { dev_err(dev, "Unable to
> register IO resource at %pR\n", res); return -EBUSY;
> }
> break;
> - case SIMATIC_IPC_DEVICE_127E:
> - res = &simatic_ipc_led_mem_res;
> - ipcled = simatic_ipc_leds_mem;
> - type = IORESOURCE_MEM;
> -
> - 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 + SZ_4K - 1;
> -
> - simatic_ipc_led_memory = devm_ioremap_resource(dev,
> res);
> - if (IS_ERR(simatic_ipc_led_memory))
> - return PTR_ERR(simatic_ipc_led_memory);
> -
> - /* initialize power/watchdog LED */
> - reg = simatic_ipc_led_memory + 0x500 + 0x1D8; /*
> PM_WDT_OUT */
> - val = readl(reg);
> - writel(val & ~1, reg);
> -
> - reg = simatic_ipc_led_memory + 0x500 + 0x1C0; /*
> PM_BIOS_BOOT_N */
> - val = readl(reg);
> - writel(val | 1, reg);
> - break;
> default:
> return -ENODEV;
> }
>
> while (ipcled->value) {
> cdev = &ipcled->cdev;
> - if (type == IORESOURCE_MEM) {
> - cdev->brightness_set =
> simatic_ipc_led_set_mem;
> - cdev->brightness_get =
> simatic_ipc_led_get_mem;
> - } else {
> - cdev->brightness_set =
> simatic_ipc_led_set_io;
> - cdev->brightness_get =
> simatic_ipc_led_get_io;
> - }
> + cdev->brightness_set = simatic_ipc_led_set_io;
> + cdev->brightness_get = simatic_ipc_led_get_io;
> cdev->max_brightness = LED_ON;
> cdev->name = ipcled->name;
>
> diff --git a/drivers/platform/x86/simatic-ipc.c
> b/drivers/platform/x86/simatic-ipc.c index 26c35e1660cb..ca3647b751d5
> 100644 --- a/drivers/platform/x86/simatic-ipc.c
> +++ b/drivers/platform/x86/simatic-ipc.c
> @@ -51,6 +51,7 @@ static int register_platform_devices(u32 station_id)
> {
> u8 ledmode = SIMATIC_IPC_DEVICE_NONE;
> u8 wdtmode = SIMATIC_IPC_DEVICE_NONE;
> + char *pdevname = KBUILD_MODNAME "_leds";
> int i;
>
> platform_data.devmode = SIMATIC_IPC_DEVICE_NONE;
> @@ -64,10 +65,12 @@ static int register_platform_devices(u32
> station_id) }
>
> if (ledmode != SIMATIC_IPC_DEVICE_NONE) {
> + if (ledmode == SIMATIC_IPC_DEVICE_127E)
> + pdevname = KBUILD_MODNAME "_leds_gpio";
> platform_data.devmode = ledmode;
> ipc_led_platform_device =
> platform_device_register_data(NULL,
> - KBUILD_MODNAME "_leds",
> PLATFORM_DEVID_NONE,
> + pdevname, PLATFORM_DEVID_NONE,
> &platform_data,
> sizeof(struct simatic_ipc_platform));
> if (IS_ERR(ipc_led_platform_device))

2022-08-02 11:01:20

by Henning Schild

[permalink] [raw]

Subject: Re: [PATCH v6 12/12] leds: simatic-ipc-leds-gpio: add GPIO version of Siemens driver

Am Mon, 1 Aug 2022 16:09:48 +0200
schrieb Henning Schild <[email protected]>:

> Am Mon, 6 Jun 2022 19:41:38 +0300
> schrieb Andy Shevchenko <[email protected]>:
>
> > From: Henning Schild <[email protected]>
> >
> > On Apollo Lake the pinctrl drivers will now come up without ACPI.
> > Use that instead of open coding it.
> > Create a new driver for that which can later be filled with more
> > GPIO based models, and which has different dependencies.
> >
> > Signed-off-by: Henning Schild <[email protected]>
> > Signed-off-by: Andy Shevchenko <[email protected]>
> > ---
> > drivers/leds/simple/Kconfig | 7 +-
> > drivers/leds/simple/Makefile | 1 +
> > drivers/leds/simple/simatic-ipc-leds-gpio.c | 105
> > ++++++++++++++++++++ drivers/leds/simple/simatic-ipc-leds.c |
> > 80 +-------------- drivers/platform/x86/simatic-ipc.c | 5
> > +- 5 files changed, 117 insertions(+), 81 deletions(-)
> > create mode 100644 drivers/leds/simple/simatic-ipc-leds-gpio.c
> >
> > diff --git a/drivers/leds/simple/Kconfig
> > b/drivers/leds/simple/Kconfig index bbf8cff3c3f6..fd2b8225d926
> > 100644 --- a/drivers/leds/simple/Kconfig
> > +++ b/drivers/leds/simple/Kconfig
> > @@ -1,12 +1,11 @@
> > # SPDX-License-Identifier: GPL-2.0-only
> > config LEDS_SIEMENS_SIMATIC_IPC
> > tristate "LED driver for Siemens Simatic IPCs"
> > - depends on LEDS_CLASS
> > + depends on LEDS_GPIO

depends on PINCTRL_BROXTON

Will have to see how to handle the additional boards. Either a gpio
driver per pinctrl provider with a clear dep chain, or some sort up
runtime check like i described in the earlier mail. Or simply depend on
all potential providers in one driver, even if on a given board only
one will be used.

Henning

> > depends on SIEMENS_SIMATIC_IPC
> > - select P2SB
> > help
> > This option enables support for the LEDs of several
> > Industrial PCs from Siemens.
> >
> > - To compile this driver as a module, choose M here: the
> > module
> > - will be called simatic-ipc-leds.
> > + To compile this driver as a module, choose M here: the
> > modules
> > + will be called simatic-ipc-leds and
> > simatic-ipc-leds-gpio. diff --git a/drivers/leds/simple/Makefile
> > b/drivers/leds/simple/Makefile index 8481f1e9e360..1c7ef5e1324b
> > 100644 --- a/drivers/leds/simple/Makefile
> > +++ b/drivers/leds/simple/Makefile
> > @@ -1,2 +1,3 @@
> > # SPDX-License-Identifier: GPL-2.0
> > obj-$(CONFIG_LEDS_SIEMENS_SIMATIC_IPC) += simatic-ipc-leds.o
> > +obj-$(CONFIG_LEDS_SIEMENS_SIMATIC_IPC) +=
> > simatic-ipc-leds-gpio.o diff --git
> > a/drivers/leds/simple/simatic-ipc-leds-gpio.c
> > b/drivers/leds/simple/simatic-ipc-leds-gpio.c new file mode 100644
> > index 000000000000..4c9e663a90ba --- /dev/null
> > +++ b/drivers/leds/simple/simatic-ipc-leds-gpio.c
> > @@ -0,0 +1,105 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Siemens SIMATIC IPC driver for GPIO based LEDs
> > + *
> > + * Copyright (c) Siemens AG, 2022
> > + *
> > + * Authors:
> > + * Henning Schild <[email protected]>
> > + */
> > +
> > +#include <linux/gpio/machine.h>
> > +#include <linux/gpio/consumer.h>
> > +#include <linux/leds.h>
> > +#include <linux/module.h>
> > +#include <linux/platform_device.h>
> > +
> > +static struct gpiod_lookup_table simatic_ipc_led_gpio_table = {
> > + .dev_id = "leds-gpio",
> > + .table = {
> > + GPIO_LOOKUP_IDX("apollolake-pinctrl.0", 51, NULL,
> > 0, GPIO_ACTIVE_LOW),
> > + GPIO_LOOKUP_IDX("apollolake-pinctrl.0", 52, NULL,
> > 1, GPIO_ACTIVE_LOW),
> > + GPIO_LOOKUP_IDX("apollolake-pinctrl.0", 53, NULL,
> > 2, GPIO_ACTIVE_LOW),
> > + GPIO_LOOKUP_IDX("apollolake-pinctrl.0", 57, NULL,
> > 3, GPIO_ACTIVE_LOW),
> > + GPIO_LOOKUP_IDX("apollolake-pinctrl.0", 58, NULL,
> > 4, GPIO_ACTIVE_LOW),
> > + GPIO_LOOKUP_IDX("apollolake-pinctrl.0", 60, NULL,
> > 5, GPIO_ACTIVE_LOW),
> > + GPIO_LOOKUP_IDX("apollolake-pinctrl.0", 56, NULL,
> > 6, GPIO_ACTIVE_LOW),
> > + GPIO_LOOKUP_IDX("apollolake-pinctrl.0", 59, NULL,
> > 7, GPIO_ACTIVE_HIGH),
> > + },
> > +};
> > +
> > +static const struct gpio_led simatic_ipc_gpio_leds[] = {
> > + { .name = "green:" LED_FUNCTION_STATUS "-3" },
> > + { .name = "red:" LED_FUNCTION_STATUS "-1" },
> > + { .name = "green:" LED_FUNCTION_STATUS "-1" },
> > + { .name = "red:" LED_FUNCTION_STATUS "-2" },
> > + { .name = "green:" LED_FUNCTION_STATUS "-2" },
> > + { .name = "red:" LED_FUNCTION_STATUS "-3" },
> > +};
> > +
> > +static const struct gpio_led_platform_data
> > simatic_ipc_gpio_leds_pdata = {
> > + .num_leds = ARRAY_SIZE(simatic_ipc_gpio_leds),
> > + .leds = simatic_ipc_gpio_leds,
> > +};
> > +
> > +static struct platform_device *simatic_leds_pdev;
> > +
> > +static int simatic_ipc_leds_gpio_remove(struct platform_device
> > *pdev) +{
> > + gpiod_remove_lookup_table(&simatic_ipc_led_gpio_table);
> > + platform_device_unregister(simatic_leds_pdev);
> > +
> > + return 0;
> > +}
> > +
> > +static int simatic_ipc_leds_gpio_probe(struct platform_device
> > *pdev) +{
> > + struct gpio_desc *gpiod;
> > + int err;
> > +
> > + gpiod_add_lookup_table(&simatic_ipc_led_gpio_table);
> > + simatic_leds_pdev = platform_device_register_resndata(NULL,
> > + "leds-gpio", PLATFORM_DEVID_NONE, NULL, 0,
> > + &simatic_ipc_gpio_leds_pdata,
> > + sizeof(simatic_ipc_gpio_leds_pdata));
>
> If those GPIOs can not be found that leads to a pretty severe
> logging/polling endless loop.
>
> ...
> [ 34.017017] leds-gpio leds-gpio: Skipping unavailable LED gpio 0
> (red:status-2) [ 34.017038] leds-gpio leds-gpio: cannot find GPIO
> chip apollolake-pinctrl.0, deferring [ 34.017158] leds-gpio
> leds-gpio: Skipping unavailable LED gpio 0 (red:status-2) [
> 34.017161] leds-gpio leds-gpio: cannot find GPIO chip
> apollolake-pinctrl.0, deferring [ 34.017163] leds-gpio leds-gpio:
> Skipping unavailable LED gpio 0 (green:status-2) [ 34.017166]
> leds-gpio leds-gpio: cannot find GPIO chip apollolake-pinctrl.0,
> deferring [ 34.017168] leds-gpio leds-gpio: Skipping unavailable
> LED gpio 0 (red:status-3) [ 34.017179] leds-gpio leds-gpio: cannot
> find GPIO chip apollolake-pinctrl.0, deferring [ 34.017275]
> leds-gpio leds-gpio: cannot find GPIO chip apollolake-pinctrl.0,
> deferring [ 34.017279] leds-gpio leds-gpio: Skipping unavailable
> LED gpio 0 (green:status-3) ...
>
> This can be seen when the kernel is build without
> CONFIG_PINCTRL_BROXTON. Can anyone please give advise on how to
> proceed here.
>
> I could do something like i proposed in "[PATCH 3/4] leds:
> simatic-ipc-leds-gpio: add new model 227G".
>
> if (!IS_ENABLED(CONFIG_PINCTRL_BROXTON)) return -ENO...
>
> before registering the platform device.
>
> An alternative could be to try and handle that -EPROBE_DEFER but that
> might be hard because it might well fail a couple of times before
> eventually that gpio driver is up, so i think i will send an update
> implementing that !IS_ENABLED.
>
> Ideally the gpio subsystem or leds-gpio would break that
> polling/printing loop for me, or make sure it would not happen in the
> first place.
>
> regards,
> Henning
>
> > + if (IS_ERR(simatic_leds_pdev)) {
> > + err = PTR_ERR(simatic_leds_pdev);
> > + goto out;
> > + }
> > +
> > + /* PM_BIOS_BOOT_N */
> > + gpiod = gpiod_get_index(&simatic_leds_pdev->dev, NULL, 6,
> > GPIOD_OUT_LOW);
> > + if (IS_ERR(gpiod)) {
> > + err = PTR_ERR(gpiod);
> > + goto out;
> > + }
> > + gpiod_put(gpiod);
> > +
> > + /* PM_WDT_OUT */
> > + gpiod = gpiod_get_index(&simatic_leds_pdev->dev, NULL, 7,
> > GPIOD_OUT_LOW);
> > + if (IS_ERR(gpiod)) {
> > + err = PTR_ERR(gpiod);
> > + goto out;
> > + }
> > + gpiod_put(gpiod);
> > +
> > + return 0;
> > +out:
> > + simatic_ipc_leds_gpio_remove(pdev);
> > +
> > + return err;
> > +}
> > +
> > +static struct platform_driver simatic_ipc_led_gpio_driver = {
> > + .probe = simatic_ipc_leds_gpio_probe,
> > + .remove = simatic_ipc_leds_gpio_remove,
> > + .driver = {
> > + .name = KBUILD_MODNAME,
> > + }
> > +};
> > +module_platform_driver(simatic_ipc_led_gpio_driver);
> > +
> > +MODULE_LICENSE("GPL v2");
> > +MODULE_ALIAS("platform:" KBUILD_MODNAME);
> > +MODULE_SOFTDEP("pre: platform:leds-gpio");
> > +MODULE_AUTHOR("Henning Schild <[email protected]>");
> > diff --git a/drivers/leds/simple/simatic-ipc-leds.c
> > b/drivers/leds/simple/simatic-ipc-leds.c index
> > 2e7597c143d8..4894c228c165 100644 ---
> > a/drivers/leds/simple/simatic-ipc-leds.c +++
> > b/drivers/leds/simple/simatic-ipc-leds.c @@ -15,7 +15,6 @@
> > #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>
> > @@ -24,7 +23,7 @@
> > #define SIMATIC_IPC_LED_PORT_BASE 0x404E
> >
> > struct simatic_ipc_led {
> > - unsigned int value; /* mask for io and offset for mem */
> > + unsigned int value; /* mask for io */
> > char *name;
> > struct led_classdev cdev;
> > };
> > @@ -39,21 +38,6 @@ static struct simatic_ipc_led
> > simatic_ipc_leds_io[] = { { }
> > };
> >
> > -/* the actual start will be discovered with p2sb, 0 is a
> > placeholder */ -static struct resource simatic_ipc_led_mem_res =
> > DEFINE_RES_MEM_NAMED(0, 0, KBUILD_MODNAME); -
> > -static void __iomem *simatic_ipc_led_memory;
> > -
> > -static struct simatic_ipc_led simatic_ipc_leds_mem[] = {
> > - {0x500 + 0x1A0, "red:" LED_FUNCTION_STATUS "-1"},
> > - {0x500 + 0x1A8, "green:" LED_FUNCTION_STATUS "-1"},
> > - {0x500 + 0x1C8, "red:" LED_FUNCTION_STATUS "-2"},
> > - {0x500 + 0x1D0, "green:" LED_FUNCTION_STATUS "-2"},
> > - {0x500 + 0x1E0, "red:" LED_FUNCTION_STATUS "-3"},
> > - {0x500 + 0x198, "green:" LED_FUNCTION_STATUS "-3"},
> > - { }
> > -};
> > -
> > static struct resource simatic_ipc_led_io_res =
> > DEFINE_RES_IO_NAMED(SIMATIC_IPC_LED_PORT_BASE, SZ_2,
> > KBUILD_MODNAME);
> > @@ -89,28 +73,6 @@ static enum led_brightness
> > simatic_ipc_led_get_io(struct led_classdev *led_cd) return
> > inw(SIMATIC_IPC_LED_PORT_BASE) & led->value ? LED_OFF :
> > led_cd->max_brightness; }
> > -static void simatic_ipc_led_set_mem(struct led_classdev *led_cd,
> > - enum led_brightness brightness)
> > -{
> > - struct simatic_ipc_led *led = cdev_to_led(led_cd);
> > - void __iomem *reg = simatic_ipc_led_memory + led->value;
> > - u32 val;
> > -
> > - val = readl(reg);
> > - val = (val & ~1) | (brightness == LED_OFF);
> > - writel(val, reg);
> > -}
> > -
> > -static enum led_brightness simatic_ipc_led_get_mem(struct
> > led_classdev *led_cd) -{
> > - struct simatic_ipc_led *led = cdev_to_led(led_cd);
> > - void __iomem *reg = simatic_ipc_led_memory + led->value;
> > - u32 val;
> > -
> > - val = readl(reg);
> > - return (val & 1) ? LED_OFF : led_cd->max_brightness;
> > -}
> > -
> > static int simatic_ipc_leds_probe(struct platform_device *pdev)
> > {
> > const struct simatic_ipc_platform *plat =
> > pdev->dev.platform_data; @@ -118,9 +80,7 @@ static int
> > simatic_ipc_leds_probe(struct platform_device *pdev) struct
> > simatic_ipc_led *ipcled; struct led_classdev *cdev;
> > struct resource *res;
> > - void __iomem *reg;
> > - int err, type;
> > - u32 val;
> > + int err;
> >
> > switch (plat->devmode) {
> > case SIMATIC_IPC_DEVICE_227D:
> > @@ -135,51 +95,19 @@ static int simatic_ipc_leds_probe(struct
> > platform_device *pdev) }
> > ipcled = simatic_ipc_leds_io;
> > }
> > - type = IORESOURCE_IO;
> > if (!devm_request_region(dev, res->start,
> > resource_size(res), KBUILD_MODNAME)) { dev_err(dev, "Unable to
> > register IO resource at %pR\n", res); return -EBUSY;
> > }
> > break;
> > - case SIMATIC_IPC_DEVICE_127E:
> > - res = &simatic_ipc_led_mem_res;
> > - ipcled = simatic_ipc_leds_mem;
> > - type = IORESOURCE_MEM;
> > -
> > - 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 + SZ_4K - 1;
> > -
> > - simatic_ipc_led_memory = devm_ioremap_resource(dev,
> > res);
> > - if (IS_ERR(simatic_ipc_led_memory))
> > - return PTR_ERR(simatic_ipc_led_memory);
> > -
> > - /* initialize power/watchdog LED */
> > - reg = simatic_ipc_led_memory + 0x500 + 0x1D8; /*
> > PM_WDT_OUT */
> > - val = readl(reg);
> > - writel(val & ~1, reg);
> > -
> > - reg = simatic_ipc_led_memory + 0x500 + 0x1C0; /*
> > PM_BIOS_BOOT_N */
> > - val = readl(reg);
> > - writel(val | 1, reg);
> > - break;
> > default:
> > return -ENODEV;
> > }
> >
> > while (ipcled->value) {
> > cdev = &ipcled->cdev;
> > - if (type == IORESOURCE_MEM) {
> > - cdev->brightness_set =
> > simatic_ipc_led_set_mem;
> > - cdev->brightness_get =
> > simatic_ipc_led_get_mem;
> > - } else {
> > - cdev->brightness_set =
> > simatic_ipc_led_set_io;
> > - cdev->brightness_get =
> > simatic_ipc_led_get_io;
> > - }
> > + cdev->brightness_set = simatic_ipc_led_set_io;
> > + cdev->brightness_get = simatic_ipc_led_get_io;
> > cdev->max_brightness = LED_ON;
> > cdev->name = ipcled->name;
> >
> > diff --git a/drivers/platform/x86/simatic-ipc.c
> > b/drivers/platform/x86/simatic-ipc.c index
> > 26c35e1660cb..ca3647b751d5 100644 ---
> > a/drivers/platform/x86/simatic-ipc.c +++
> > b/drivers/platform/x86/simatic-ipc.c @@ -51,6 +51,7 @@ static int
> > register_platform_devices(u32 station_id) {
> > u8 ledmode = SIMATIC_IPC_DEVICE_NONE;
> > u8 wdtmode = SIMATIC_IPC_DEVICE_NONE;
> > + char *pdevname = KBUILD_MODNAME "_leds";
> > int i;
> >
> > platform_data.devmode = SIMATIC_IPC_DEVICE_NONE;
> > @@ -64,10 +65,12 @@ static int register_platform_devices(u32
> > station_id) }
> >
> > if (ledmode != SIMATIC_IPC_DEVICE_NONE) {
> > + if (ledmode == SIMATIC_IPC_DEVICE_127E)
> > + pdevname = KBUILD_MODNAME "_leds_gpio";
> > platform_data.devmode = ledmode;
> > ipc_led_platform_device =
> > platform_device_register_data(NULL,
> > - KBUILD_MODNAME "_leds",
> > PLATFORM_DEVID_NONE,
> > + pdevname, PLATFORM_DEVID_NONE,
> > &platform_data,
> > sizeof(struct
> > simatic_ipc_platform)); if (IS_ERR(ipc_led_platform_device))
>

2022-08-10 08:28:25

by Henning Schild

[permalink] [raw]

Subject: Re: [PATCH v6 00/12] platform/x86: introduce p2sb_bar() helper

Am Mon, 18 Jul 2022 14:17:42 +0300
schrieb Andy Shevchenko <[email protected]>:

> On Mon, Jun 06, 2022 at 07:41:26PM +0300, Andy Shevchenko wrote:
> > There are a few users 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 to 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 brings 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.
> >
> > 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).
> >
> > The series is ready to be merged via MFD tree, but see below.
> >
> > The series also includes updates for Simatic IPC drivers that
> > partially tagged by respective maintainers (the main question is if
> > Pavel is okay with the last three patches, since I believe Hans is
> > okay with removing some code under PDx86). Hence the first 8
> > patches can be merged right away and the rest when Pavel does his
> > review.
>
> Kernel test bot seems found an issue with dependencies, because
> selection of P2SB is not enough.
>
> There are two solutions that I can see now:
> 1) move P2SB out of X86_PLATFORM_DEVICES section (like PMC_ATOM);
> 2) add 'depends on X86_PLATFORM_DEVICES' to the affected drivers.
>
> I think the first solution cleaner, because it would be strange to
> have the dependency on the drivers that quite unlikely be on server
> platforms (e.g. EDAC).
>
> In long term perhaps something like drivers/platform/x86/lib which is
> for platform libraries or so and independent on X86_PLATFORM_DEVICES?
>
> I will send a fix soon as per 1) above, feel free to comment here or
> there.

Hey Andy,

is that one on the way already? I meanwhile also found a possible
configuration issue in my patches you carry on top. And i suggest to
include or squash
[PATCH] leds: simatic-ipc-leds-gpio: make sure we have the GPIO
providing driver

in this series.

I am also working on adding more models which have GPIO based LEDs. All
the patches are based on this series because it is where i currently
introduce GPIO based LEDs for simatic. I would not want to change the
ordering but at the same time i would like to meet 5.20.

regards,
Henning