Messages in this thread | | | Date | Wed, 23 Aug 2023 10:51:11 +0200 | From | neil.armstrong@linaro ... | Subject | Re: [PATCH v2] irqchip/qcom-pdc: add support for v3.2 HW |
| |
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 >
| |