Re: [PATCH v2 5/8] irqchip/qcom-pdc: Configure PDC to pass through mode

[Stephan Gerhold <stephan.gerhold@linaro.org>]

On Tue, May 26, 2026 at 04:24:41PM +0530, Maulik Shah wrote:
> All PDC irqchip supports pass through mode in which both Direct SPIs and
> GPIO IRQs (as SPIs) are sent to GIC without latching at PDC.
> 
> Newer PDCs (v3.0 onwards) also support additional secondary controller mode
> where PDC latches GPIO IRQs and sends to GIC as level type IRQ. Direct SPIs
> still works same as pass through mode without latching at PDC even in
> secondary controller mode.
> 
> All the SoCs so far default uses pass through mode with the exception of
> x1e. x1e PDC may be set to secondary controller mode for builds on CRD
> boards whereas it may be set to pass through mode for IoT-EVK boards.
> The mode configuration is done in firmware and initially shipped windows
> firmware did not have SCM interface to read or modify the PDC mode.
> Later only write access is opened up for non secure world.
> 
> Using the write access available add changes to modify the PDC mode to
> pass through mode via SCM write. When the write fails (on older firmware)
> assume to work in secondary mode.
> 
> Co-developed-by: Sneh Mankad <sneh.mankad@oss.qualcomm.com>
> Signed-off-by: Sneh Mankad <sneh.mankad@oss.qualcomm.com>
> Signed-off-by: Maulik Shah <maulik.shah@oss.qualcomm.com>
> ---
>  drivers/irqchip/qcom-pdc.c | 109 +++++++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 106 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/irqchip/qcom-pdc.c b/drivers/irqchip/qcom-pdc.c
> index 86379dddc5be..69ddd7d89a83 100644
> --- a/drivers/irqchip/qcom-pdc.c
> +++ b/drivers/irqchip/qcom-pdc.c
> [...]
> @@ -135,6 +151,8 @@ static const struct pdc_regs pdc_v3_2 = {
>  };
>  
>  static const struct pdc_cfg pdc_cfg_v3_2 = {
> +	.gpio_irq_sts = GENMASK(5, 5),
> +	.gpio_irq_mask = GENMASK(4, 4),

BIT(5) / BIT(4) would be clearer here in my opinion.

>  	.irq_enable = GENMASK(3, 3),
>  	.irq_type = GENMASK(2, 0),
>  };
> [...]
> @@ -184,6 +204,14 @@ static u32 pdc_reg_read(int reg, u32 i)
>  	return readl_relaxed(pdc->base + reg + i * sizeof(u32));
>  }
>  
> +static inline bool pdc_pin_uses_seconary_mode(int pin_out)
> +{
> +	if (pdc->mode == PDC_SECONDARY_MODE && pin_out >= pdc->num_spis)
> +		return true;
> +
> +	return false;

Can put this in one line:

	return pdc->mode == PDC_SECONDARY_MODE && pin_out >= pdc->num_spis;

> +}
> +
>  static void pdc_x1e_irq_enable_write(u32 bank, u32 enable)
>  {
>  	void __iomem *base;
> @@ -232,6 +260,36 @@ static void pdc_enable_intr_bank(int pin_out, bool on)
>  		pdc_reg_write(pdc->regs->irq_en_reg, index, enable);
>  
>  	raw_spin_unlock_irqrestore(&pdc->lock, flags);
> +
> +	if (pdc_pin_uses_seconary_mode(pin_out))
> +		pdc->unmask_gpio(pin_out, on);
> +}
> +
> +static void pdc_clear_gpio_cfg(int pin_out)
> +{
> +	unsigned long gpio_sts;
> +
> +	if (pdc->version < PDC_VERSION_3_0)
> +		return;
> +
> +	gpio_sts = pdc_reg_read(pdc->regs->irq_cfg_reg, pin_out);
> +	gpio_sts &= ~pdc->cfg->gpio_irq_sts;
> +	pdc_reg_write(pdc->regs->irq_cfg_reg, pin_out, gpio_sts);

Is this guaranteed to be called sequentially, i.e. not in parallel on
another CPU? Otherwise, you need to add the lock here to make sure the
read-modify-write doesn't race with another CPU.

Note that since the irq_cfg_reg is also used in qcom_pdc_gic_set_type()
it would be safest to add the lock there as well (although since PDC has
IRQCHIP_SET_TYPE_MASKED it's probably unlikely to be called in parallel
with another irqchip operation for the same IRQ). In my patch, I handled
this for all users using a new pdc_update_irq_cfg() function [1].

[1]: https://github.com/stephan-gh/linux/commit/59ca2a7335ede83e4a7cf02704dd7c469c725c14

> +}
> +
> +static void pdc_unmask_gpio_cfg(int pin_out, bool unmask)
> +{
> +	unsigned long gpio_mask;
> +
> +	if (pdc->version < PDC_VERSION_3_0)
> +		return;
> +
> +	gpio_mask = pdc_reg_read(pdc->regs->irq_cfg_reg, pin_out);
> +	if (unmask)
> +		gpio_mask &= ~pdc->cfg->gpio_irq_mask;
> +	else
> +		gpio_mask |= pdc->cfg->gpio_irq_mask;
> +	pdc_reg_write(pdc->regs->irq_cfg_reg, pin_out, gpio_mask);
>  }
>  
>  static void pdc_enable_intr_cfg(int pin_out, bool on)
> [...]
> @@ -258,6 +319,20 @@ static void qcom_pdc_gic_enable(struct irq_data *d)
>  	irq_chip_enable_parent(d);
>  }
>  
> +static void qcom_pdc_ack(struct irq_data *d)
> +{
> +	if (pdc_pin_uses_seconary_mode(d->hwirq) && !irqd_is_level_type(d))
> +		pdc->clear_gpio(d->hwirq);
> +}

You might need a write memory barrier here and/or read-back here to make
sure the write is complete before the interrupt is unmasked in the GIC.
IIRC I added this in my patch after seeing some test tlmm-test failure..

> +
> +static void qcom_pdc_gic_eoi(struct irq_data *d)
> +{
> +	if (pdc_pin_uses_seconary_mode(d->hwirq) && irqd_is_level_type(d))
> +		pdc->clear_gpio(d->hwirq);
> +
> +	irq_chip_eoi_parent(d);
> +}
> +
>  /*
>   * GIC does not handle falling edge or active low. To allow falling edge and
>   * active low interrupts to be handled at GIC, PDC has an inverter that inverts
> [...]
> @@ -510,6 +600,10 @@ static int qcom_pdc_probe(struct platform_device *pdev, struct device_node *pare
>  		pdc->enable_intr = pdc_enable_intr_bank;
>  	}
>  
> +	pdc->unmask_gpio = pdc_unmask_gpio_cfg;
> +	pdc->clear_gpio = pdc_clear_gpio_cfg;

What is the purpose of these function pointers if you always assign the
same function?

Thanks,
Stephan