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

["Maulik Shah (mkshah)" <maulik.shah@oss.qualcomm.com>]

On 5/26/2026 5:52 PM, Stephan Gerhold wrote:
> On Tue, May 26, 2026 at 04:24:41PM +0530, Maulik Shah wrote:

[...]

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

GENMASK gives uniformity.

> 
>>  	.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;
> 
>> +}

Sure.

>> +

[...]

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

Right. with irq_desc->lock held it will be called sequentially and no locking
needed.

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

I did not see any need for barries and all tlmm-test passed.

[...]

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

I have updated them in v3 to be assigned only for secondary mode.

Thanks,
Maulik