On 23/08/2023 10:25, Maulik Shah (mkshah) wrote:
Hi,
On 8/23/2023 1:16 PM, Neil Armstrong wrote:
Hi,
On 23/08/2023 07:35, Maulik Shah (mkshah) wrote:
Hi Neil,
@@ -142,8 +163,17 @@ static int qcom_pdc_gic_set_type(struct irq_data *d, unsigned int type)
return -EINVAL;
}
- old_pdc_type = pdc_reg_read(IRQ_i_CFG, d->hwirq);
- pdc_reg_write(IRQ_i_CFG, d->hwirq, pdc_type);
+ if (pdc_version < PDC_VERSION_3_2) {
+ old_pdc_type = pdc_reg_read(IRQ_i_CFG, d->hwirq);
+ pdc_reg_write(IRQ_i_CFG, d->hwirq, pdc_type);
+ } else {
+ u32 val;
+
+ val = pdc_reg_read(IRQ_i_CFG, d->hwirq);
+ old_pdc_type = val & IRQ_i_CFG_TYPE_MASK;
+ pdc_reg_write(IRQ_i_CFG, d->hwirq,
+ pdc_type | (val & IRQ_i_CFG_IRQ_ENABLE));
+ }
While above is correct, i don't think we need version check in qcom_pdc_gic_set_type() as bits 0-2 are always for the type in old/new version as mentioned in v1.
Adding one line after reading old_pdc_type should be good enough.
Yes I understood, but while looking at the IRQ_i_CFG bits, I wanted to keep the original
driver behavior intact by setting remaining bits to 0.
Adding this single line changes that behavior and keeps bits 3-31
to the default register value, which may have some consequences.
If you consider it's an ok change, then I'll reduce it to this single line.
Yes this ok change to have single line and should not have any consequences.
I also remember why, it's about the final check:
184 if (old_pdc_type != pdc_type)
185 irq_chip_set_parent_state(d, IRQCHIP_STATE_PENDING, false);
We need to strip out remaining bits of old_pdc_type of this won't work as
expected, so I'll change it to :
+ old_pdc_type = pdc_reg_read(IRQ_i_CFG, d->hwirq);
+ pdc_type |= (old_pdc_type & ~IRQ_i_CFG_TYPE_MASK);
+ old_pdc_type &= IRQ_i_CFG_TYPE_MASK;
+ pdc_reg_write(IRQ_i_CFG, d->hwirq, pdc_type);
Is it ok for you ?
Thanks,
Neil
Thanks,
Maulik
