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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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





[Index of Archives]     [Linux ARM Kernel]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [Linux for Sparc]     [IETF Annouce]     [Security]     [Bugtraq]     [Linux MIPS]     [ECOS]     [Asterisk Internet PBX]     [Linux API]

  Powered by Linux