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

[Hans de Goede <johannes.goede@oss.qualcomm.com>]

On 29-Jun-26 3:59 AM, Bjorn Andersson wrote:
> 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,

The changes to pinctrl-msm.c itself are pretty minimal and the 200 lines
in pinctrl-msm-acpi.c are not too bad given that they fix a bunch of
GpioInt-s in the ACPI tables not working.

> to support a model
> that I am not convinced we want to retain - and that only works in the
> hybrid case.

With the exception of relying on the pdc-ranges from DT, this should
work fine in an ACPI only mode too and would be quite useful to have
in ACPI only mode.

Now that I think of it, why are the pdc-ranges in DT at all ?

AFAICT this is a property of the Soc, so this could just be in the pinctrl
driver based on the compatible. Just like the wakeirq_map is coded
in the driver. It feels a bit weird to have one defined in DT and
the other coded in the driver?



> 
>> 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?

Not AFAICT on my Samsung Galaxy Go with Snapdragon 7c gen 2 /
sc7180 using 64 is the right thing to do, as is on the x1e laptops.

> 
>> +#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"?

I'm using "Windows ACPI" / Woa ACPI" in comments and naming here to distinguish
this from ARM System Ready ACPI tables, for which there also seems to be some
support directly inside pinctrl-msm.c .

> 
>> +	 * 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.

Ack, see above. We could just hardcode the PDC ranges on a per compatible
bases (assuming we want this patch at all). Then this should work fine for
the pure ACPI case too.

This would actually be an interesting thing to have for people interested
in doing further experiments with a pure ACPI mode.


>> +{
>> +	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)

The ACPI Interrupt resource gives a GIC IRQ number, which needs to go through
pdc-range mapping to become a PDC pin number, which then needs to go through
soc_data->wakeirq_map to find the TLMM pin number.

Regards,

Hans