Re: [RFC 09/12] pinctrl: qcom: Add support for WoA ACPI tables virtual TLMM pin numbers

[Bjorn Andersson <andersson@kernel.org>]

On Tue, Jun 23, 2026 at 04:52:22PM +0200, Hans de Goede wrote:
> The ACPI tabled on Windows on ARM laptops use TLMM pin numbers outside of
> the actual TLMM pin number range. These are a rather convoluted way to let
> the Windows Qualcomm GPIO driver now to use the PDC for some pins because
> these are wakeup sources.
> 
> This adds support for translating the magic Windows virtual GPIOs for these
> back to a regular TLMM GPIO so that ACPI described devices using these
> virtual GPIOs can work with Linux.
> 
> For now this code only tries to do this mapping when booting in DT-ACPI
> hybrid mode which is only used on some WoA devices so this should not
> impact any other use-cases.
> 
> The new functions use woa_acpi in their name to make clear that these
> are for dealing with the ACPI tables found on WoA devices, rather then
> ACPI tables found on other devices, like ARM system ready devices which
> also use ACPI.
> 
> Note that simply mapping these virtual GPIOs back to TLMM pin numbers can
> safely be done on Linux, because Linux always uses the PDC for GPIO IRQs
> where possible.
> 

This adds a fair amount of complexity to the driver, to support a model
that I am not convinced we want to retain - and that only works in the
hybrid case.

> Signed-off-by: Hans de Goede <johannes.goede@oss.qualcomm.com>
> ---
>  drivers/pinctrl/qcom/Makefile           |   4 +-
>  drivers/pinctrl/qcom/pinctrl-msm-acpi.c | 196 ++++++++++++++++++++++++
>  drivers/pinctrl/qcom/pinctrl-msm.c      |  47 +++++-
>  drivers/pinctrl/qcom/pinctrl-msm.h      |  35 +++++
>  4 files changed, 278 insertions(+), 4 deletions(-)
>  create mode 100644 drivers/pinctrl/qcom/pinctrl-msm-acpi.c
> 
> diff --git a/drivers/pinctrl/qcom/Makefile b/drivers/pinctrl/qcom/Makefile
> index 84bda3ada874..9029d99190d2 100644
> --- a/drivers/pinctrl/qcom/Makefile
> +++ b/drivers/pinctrl/qcom/Makefile
> @@ -1,6 +1,8 @@
>  # SPDX-License-Identifier: GPL-2.0
>  # Qualcomm pin control drivers
> -obj-$(CONFIG_PINCTRL_MSM)	+= pinctrl-msm.o
> +obj-$(CONFIG_PINCTRL_MSM)	+= pinctrl-msm-core.o
> +pinctrl-msm-core-y		:= pinctrl-msm.o
> +pinctrl-msm-core-$(CONFIG_ACPI)	+= pinctrl-msm-acpi.o
>  obj-$(CONFIG_PINCTRL_APQ8064)	+= pinctrl-apq8064.o
>  obj-$(CONFIG_PINCTRL_APQ8084)	+= pinctrl-apq8084.o
>  obj-$(CONFIG_PINCTRL_ELIZA)	+= pinctrl-eliza.o
> diff --git a/drivers/pinctrl/qcom/pinctrl-msm-acpi.c b/drivers/pinctrl/qcom/pinctrl-msm-acpi.c
> new file mode 100644
> index 000000000000..df180fd04749
> --- /dev/null
> +++ b/drivers/pinctrl/qcom/pinctrl-msm-acpi.c
> @@ -0,0 +1,196 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * ACPI GPIO lookup handling for WoA (Windows on ARM) laptop ACPI tables.
> + *
> + * Copyright (c) Qualcomm Technologies, Inc. and/or its subsidiaries.
> + */
> +
> +#include <linux/acpi.h>
> +#include <linux/device.h>
> +#include <linux/dev_printk.h>
> +#include <linux/gpio/driver.h>
> +#include <linux/list.h>
> +#include <linux/math.h>
> +#include "pinctrl-msm.h"
> +
> +#define MSM_GPIO_WOA_ACPI_GPIOS_PER_BANK	64

Wasn't this 32 for a while?

> +#define MSM_GPIO_WOA_ACPI_IRQ_OFFSET		32
> +#define MSM_GPIO_WOA_ACPI_INVALID_GPIO		~0U
> +#define MSM_GPIO_WOA_ACPI_MAX_PDC_RANGES	16
> +
> +#define PDC_RANGE_PIN_BASE			0
> +#define PDC_RANGE_GIC_BASE			1
> +#define PDC_RANGE_COUNT				2
> +#define PDC_RANGE_ELEMENTS			3
> +
> +/**
> + * struct msm_gpio_woa_acpi_parse_data - Data for parsing WoA ACPI GPIO ctl resources
> + * @chip:		gpiochip handle
> + * @data:		Data for mapping virtual WoA ACPI PDC IRQ GPIOs
> + * @soc_data:		Reference to soc_data of platform specific data
> + * @pdc_range:		PDC GIC to PDC map ranges
> + * @pdc_range_count:	PDC GIC to PDC map range-count
> + */
> +struct msm_gpio_woa_acpi_parse_data {
> +	struct gpio_chip *chip;
> +	struct msm_gpio_woa_acpi_data *data;
> +	const struct msm_pinctrl_soc_data *soc_data;
> +	u32 pdc_range[MSM_GPIO_WOA_ACPI_MAX_PDC_RANGES][PDC_RANGE_ELEMENTS];
> +	unsigned int pdc_range_count;
> +};
> +
> +/*
> + * Mapping does not need translating the acpi_resource in to a regular resoure
> + * and adding it to the resource list. Always return 1 to disable this.
> + */
> +static int msm_gpio_woa_acpi_resource(struct acpi_resource *ares, void *_parse)
> +{
> +	struct msm_gpio_woa_acpi_parse_data *parse = _parse;
> +	const struct msm_pinctrl_soc_data *soc_data = parse->soc_data;
> +	struct msm_gpio_woa_acpi_data *data = parse->data;
> +	struct gpio_chip *chip = parse->chip;
> +	u32 gic_irq, pdc_pin;
> +
> +	if (ares->type != ACPI_RESOURCE_TYPE_EXTENDED_IRQ ||
> +	    ares->data.extended_irq.interrupt_count != 1)
> +		return 1;
> +
> +	if (data->nmap == MSM_GPIO_WOA_ACPI_MAX_VIRT_GPIOS) {
> +		dev_err(chip->parent, "ACPI resources contain more than %d IRQs\n",
> +			MSM_GPIO_WOA_ACPI_MAX_VIRT_GPIOS);
> +		return 1;
> +	}
> +
> +	/*
> +	 * Windows ACPI tables divide GPIOs into banks of 64 pins with one IRQ

Is this really "Windows ACPI tables"?

> +	 * per bank. The resources start with listing the real TLMM IRQ for
> +	 * as many banks as are necessary to cover the real GPIOs. The Windows
> +	 * virtual GPIO indexes skip these banks, mark them as unavailable.
> +	 */
> +	if (data->nmap < DIV_ROUND_UP(chip->ngpio, MSM_GPIO_WOA_ACPI_GPIOS_PER_BANK)) {
> +		data->map[data->nmap++] = MSM_GPIO_WOA_ACPI_INVALID_GPIO;
> +		return 1;
> +	}
> +
> +	/*
> +	 * Use the "pdc-ranges" property on the PDC to translate the GIC IRQ
> +	 * from the acpi_resource to a PDC pin.
> +	 */
> +	gic_irq = ares->data.extended_irq.interrupts[0] - MSM_GPIO_WOA_ACPI_IRQ_OFFSET;
> +	pdc_pin = MSM_GPIO_WOA_ACPI_INVALID_GPIO;
> +	for (unsigned int i = 0; i < parse->pdc_range_count; i++) {
> +		u32 gic_base = parse->pdc_range[i][PDC_RANGE_GIC_BASE];
> +		u32 count = parse->pdc_range[i][PDC_RANGE_COUNT];
> +		if (gic_irq >= gic_base && gic_irq < (gic_base + count)) {
> +			pdc_pin = parse->pdc_range[i][PDC_RANGE_PIN_BASE] +
> +				  gic_irq - gic_base;
> +			break;
> +		}
> +	}
> +	if (pdc_pin == MSM_GPIO_WOA_ACPI_INVALID_GPIO)
> +		goto no_map;
> +
> +	/* Use wakeirq-map to map PDC pin to TLMM pin */
> +	for (unsigned int i = 0; i < soc_data->nwakeirq_map; i++) {
> +		if (soc_data->wakeirq_map[i].wakeirq == pdc_pin) {
> +			data->map[data->nmap++] = soc_data->wakeirq_map[i].gpio;
> +			return 1;
> +		}
> +	}
> +
> +no_map:
> +	dev_warn(chip->parent, "Cannot map GIC IRQ %u to TLMM pin\n", gic_irq);
> +	data->map[data->nmap++] = MSM_GPIO_WOA_ACPI_INVALID_GPIO;
> +	return 1;
> +}
> +
> +int msm_gpio_woa_acpi_init(struct gpio_chip *chip, struct msm_gpio_woa_acpi_data *data,
> +			   const struct msm_pinctrl_soc_data *soc_data)

This function name makes me think this deals with "the ACPI case", but
it requires both ACPI and DT tables to define the TLMM block - in other
words, it complicates the DT-only case and it's useless in a ACPI-only
system.

> +{
> +	struct msm_gpio_woa_acpi_parse_data parse;
> +	struct fwnode_handle *fwnode;
> +	struct device_node *pdc_np;
> +	LIST_HEAD(resources);
> +	unsigned int ngpio;
> +	int ret;
> +
> +	/* WoA ACPI tables are only used in DT-ACPI hybrid mode */
> +	fwnode = chip->parent->fwnode;
> +	if (!is_of_node(fwnode) || !is_acpi_device_node(fwnode->secondary))
> +		return 0;
> +
> +	parse.chip = chip;
> +	parse.data = data;
> +	parse.soc_data = soc_data;
> +
> +	/* Get PDC ranges, the PDC is the TLMM's wakeup-parent. */
> +	pdc_np = of_parse_phandle(chip->parent->of_node, "wakeup-parent", 0);
> +	if (!pdc_np)
> +		return 0;
> +
> +	ret = of_property_count_elems_of_size(pdc_np, "qcom,pdc-ranges", sizeof(u32));

That said, do you actually need to do this? Doesn't the ACPI resource
give you the INTID directly? (Perhaps I'm misremember? Or perhaps that's
of no use to us)

Regards,
Bjorn