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

[Konrad Dybcio]

On 6/16/26 11:25 AM, 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.
> 
> In secondary mode set the separate irqchip for the GPIOs to perform
> additional operations only for the GPIO irqs.
> 
> Co-developed-by: Sneh Mankad <sneh.mankad@xxxxxxxxxxxxxxxx>
> Signed-off-by: Sneh Mankad <sneh.mankad@xxxxxxxxxxxxxxxx>
> Signed-off-by: Maulik Shah <maulik.shah@xxxxxxxxxxxxxxxx>
> ---

[...]

> +static int qcom_pdc_gic_secondary_set_type(struct irq_data *d, unsigned int type)
> +{
> +	enum pdc_irq_config_bits pdc_type;
> +	enum pdc_irq_config_bits old_pdc_type;
> +	int ret;
> +
> +	switch (type) {
> +	case IRQ_TYPE_EDGE_RISING:
> +		pdc_type = PDC_EDGE_RISING;
> +		break;
> +	case IRQ_TYPE_EDGE_FALLING:
> +		pdc_type = PDC_EDGE_FALLING;
> +		break;
> +	case IRQ_TYPE_EDGE_BOTH:
> +		pdc_type = PDC_EDGE_DUAL;
> +		break;
> +	case IRQ_TYPE_LEVEL_HIGH:
> +		pdc_type = PDC_LEVEL_HIGH;
> +		break;
> +	case IRQ_TYPE_LEVEL_LOW:
> +		pdc_type = PDC_LEVEL_LOW;
> +		break;
> +	default:
> +		WARN_ON(1);
> +		return -EINVAL;
> +	}
> +
> +	old_pdc_type = pdc_reg_read(pdc->regs->irq_cfg_reg, d->hwirq);
> +	pdc_type |= (old_pdc_type & ~pdc->cfg_fields->irq_type);
> +	pdc_reg_write(pdc->regs->irq_cfg_reg, d->hwirq, pdc_type);
> +
> +	type = IRQ_TYPE_LEVEL_HIGH;


Please carry your comment from the previous revision:

/*
 * PDC forwards GPIOs as level high to GIC in secondary
 * mode. Update the type and clear any previously latched
 * phantom interrupt at PDC.
 */

> +	pdc->clear_gpio(d->hwirq);
> +
> +	ret = irq_chip_set_type_parent(d, type);
> +	if (ret)
> +		return ret;
> +
> +	/*
> +	 * When we change types the PDC can give a phantom interrupt.
> +	 * Clear it.  Specifically the phantom shows up when reconfiguring
> +	 * polarity of interrupt without changing the state of the signal
> +	 * but let's be consistent and clear it always.
> +	 *
> +	 * Doing this works because we have IRQCHIP_SET_TYPE_MASKED so the
> +	 * interrupt will be cleared before the rest of the system sees it.
> +	 */
> +	if (old_pdc_type != pdc_type)
> +		irq_chip_set_parent_state(d, IRQCHIP_STATE_PENDING, false);

This bit and the switch statement above are common between the two paths..
I'm debating whether we should factor them out as static inline void, but
neither solution is perfect.. so, up to you

[...]

> @@ -385,20 +547,37 @@ static int qcom_pdc_alloc(struct irq_domain *domain, unsigned int virq,
>  	if (hwirq == GPIO_NO_WAKE_IRQ)
>  		return irq_domain_disconnect_hierarchy(domain, virq);
>  
> -	ret = irq_domain_set_hwirq_and_chip(domain, virq, hwirq,
> -					    &qcom_pdc_gic_chip, NULL);
> -	if (ret)
> -		return ret;
> +	/*
> +	 * PDC secondary chip is only set for the GPIO interrupts as SPIs.
> +	 * Direct SPI interrupts are still in pass through mode (no latching
> +	 * at PDC).
> +	 */
> +	if (pdc->mode == PDC_PASS_THROUGH_MODE || !pdc_pin_is_gpio(hwirq)) {
> +		ret = irq_domain_set_hwirq_and_chip(domain, virq, hwirq,
> +						    &qcom_pdc_gic_chip,
> +						    NULL);
> +		if (ret)
> +			return ret;
>  
> -	region = get_pin_region(hwirq);
> -	if (!region)
> -		return irq_domain_disconnect_hierarchy(domain->parent, virq);
> +		if (type & IRQ_TYPE_EDGE_BOTH)
> +			type = IRQ_TYPE_EDGE_RISING;
>  
> -	if (type & IRQ_TYPE_EDGE_BOTH)
> -		type = IRQ_TYPE_EDGE_RISING;
> +		if (type & IRQ_TYPE_LEVEL_MASK)
> +			type = IRQ_TYPE_LEVEL_HIGH;
> +	} else {
> +		ret = irq_domain_set_hwirq_and_chip(domain, virq, hwirq,
> +						    &qcom_pdc_gic_secondary_chip,
> +						    NULL);
> +		if (ret)
> +			return ret;
>  
> -	if (type & IRQ_TYPE_LEVEL_MASK)
> +		/* Secondary mode converts all interrupts to LEVEL HIGH type */
>  		type = IRQ_TYPE_LEVEL_HIGH;
> +	}

nit: (pdc->mode == PDC_SECONDARY_MODE && pdc_pin_is_gpio(hwirq))
could be the primary case to better communicate intent

Konrad