Hi Neil,
On 8/23/2023 2:21 PM, neil.armstrong@xxxxxxxxxx wrote:
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 ?
No.
old_pdc_type = pdc_reg_read(IRQ_i_CFG, d->hwirq);
+ pdc_type |= (old_pdc_type & ~IRQ_i_CFG_TYPE_MASK);
Adding above suggested single line is sufficient to make final check
properly compare both old_pdc_type and new pdc_type, right?
But with your above change, It will end up comparing only bits 0-2 of
old_pdc_type with updated pdc_type (which just got the other bits (3 to
31) of IRQ_i_CFG register by the ORing it with old_pdc_type).
Thanks,
Maulik
