Re: [PATCH v2] irqchip/qcom-pdc: add support for v3.2 HW

From: neil . armstrong
Date: Wed Aug 23 2023 - 05:34:58 EST


On 23/08/2023 11:19, Maulik Shah (mkshah) wrote:
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).

Oh yeah indeed it's right, I had my previous code in mind.

I'll stick with the single line then,

Thanks,
Neil


Thanks,
Maulik