| Message ID | 20230829092119.1017194-1-dmitry.baryshkov@linaro.org |
|---|---|
| Headers | show |
| Series | irqchip/qcom-pdc: support v3.2 HW | expand |
On Tue, 29 Aug 2023 10:21:18 +0100, Dmitry Baryshkov <dmitry.baryshkov@linaro.org> wrote: > > From: Neil Armstrong <neil.armstrong@linaro.org> > > Starting from HW version 3.2 the IRQ_ENABLE bit has moved to the > IRQ_i_CFG register and requires a change of the driver to avoid > writing into an undefined register address. > > Get the HW version from registers and set the IRQ_ENABLE bit to the > correct register depending on the HW version. > > Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org> > [DB: fix crash on sm8150 DTs which listed short PDC region] > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> > --- > drivers/irqchip/qcom-pdc.c | 73 ++++++++++++++++++++++++++++++-------- > 1 file changed, 59 insertions(+), 14 deletions(-) > > diff --git a/drivers/irqchip/qcom-pdc.c b/drivers/irqchip/qcom-pdc.c > index a32c0d28d038..f9f44b494b1d 100644 > --- a/drivers/irqchip/qcom-pdc.c > +++ b/drivers/irqchip/qcom-pdc.c > @@ -22,9 +22,22 @@ > > #define PDC_MAX_GPIO_IRQS 256 > > +/* Valid only on HW version < 3.2 */ > #define IRQ_ENABLE_BANK 0x10 > #define IRQ_i_CFG 0x110 > > +/* Valid only on HW version >= 3.2 */ > +#define IRQ_i_CFG_IRQ_ENABLE 3 > + > +#define IRQ_i_CFG_TYPE_MASK GENMASK(2, 0) > + > +#define PDC_VERSION 0x1000 That's an offset, right? Maybe spelling it as such would make this more readable... > + > +/* Notable PDC versions */ > +enum { > + PDC_VERSION_3_2 = 0x30200, ... specially when reading this (why is it all of a sudden an enum?). > +}; > + > struct pdc_pin_region { > u32 pin_base; > u32 parent_base; > @@ -37,6 +50,7 @@ static DEFINE_RAW_SPINLOCK(pdc_lock); > static void __iomem *pdc_base; > static struct pdc_pin_region *pdc_region; > static int pdc_region_cnt; > +static unsigned int pdc_version; > > static void pdc_reg_write(int reg, u32 i, u32 val) > { > @@ -53,15 +67,22 @@ static void pdc_enable_intr(struct irq_data *d, bool on) > int pin_out = d->hwirq; > unsigned long enable; > unsigned long flags; > - u32 index, mask; > - > - index = pin_out / 32; > - mask = pin_out % 32; > > raw_spin_lock_irqsave(&pdc_lock, flags); > - enable = pdc_reg_read(IRQ_ENABLE_BANK, index); > - __assign_bit(mask, &enable, on); > - pdc_reg_write(IRQ_ENABLE_BANK, index, enable); > + if (pdc_version < PDC_VERSION_3_2) { > + u32 index, mask; > + > + index = pin_out / 32; > + mask = pin_out % 32; > + > + enable = pdc_reg_read(IRQ_ENABLE_BANK, index); > + __assign_bit(mask, &enable, on); > + pdc_reg_write(IRQ_ENABLE_BANK, index, enable); > + } else { > + enable = pdc_reg_read(IRQ_i_CFG, pin_out); > + __assign_bit(IRQ_i_CFG_IRQ_ENABLE, &enable, on); > + pdc_reg_write(IRQ_i_CFG, pin_out, enable); > + } > raw_spin_unlock_irqrestore(&pdc_lock, flags); > } > > @@ -142,6 +163,7 @@ static int qcom_pdc_gic_set_type(struct irq_data *d, unsigned int type) > } > > old_pdc_type = pdc_reg_read(IRQ_i_CFG, d->hwirq); > + pdc_type |= (old_pdc_type & ~IRQ_i_CFG_TYPE_MASK); > pdc_reg_write(IRQ_i_CFG, d->hwirq, pdc_type); > > ret = irq_chip_set_type_parent(d, type); > @@ -246,7 +268,7 @@ static const struct irq_domain_ops qcom_pdc_ops = { > static int pdc_setup_pin_mapping(struct device_node *np) > { > int ret, n, i; > - u32 irq_index, reg_index, val; > + unsigned long val; > > n = of_property_count_elems_of_size(np, "qcom,pdc-ranges", sizeof(u32)); > if (n <= 0 || n % 3) > @@ -277,28 +299,51 @@ static int pdc_setup_pin_mapping(struct device_node *np) > return ret; > > for (i = 0; i < pdc_region[n].cnt; i++) { > - reg_index = (i + pdc_region[n].pin_base) >> 5; > - irq_index = (i + pdc_region[n].pin_base) & 0x1f; > - val = pdc_reg_read(IRQ_ENABLE_BANK, reg_index); > - val &= ~BIT(irq_index); > - pdc_reg_write(IRQ_ENABLE_BANK, reg_index, val); > + if (pdc_version < PDC_VERSION_3_2) { > + u32 irq_index, reg_index; > + > + reg_index = (i + pdc_region[n].pin_base) >> 5; > + irq_index = (i + pdc_region[n].pin_base) & 0x1f; > + val = pdc_reg_read(IRQ_ENABLE_BANK, reg_index); > + __assign_bit(irq_index, &val, 0); > + pdc_reg_write(IRQ_ENABLE_BANK, reg_index, val); > + } else { > + u32 irq; > + > + irq = i + pdc_region[n].pin_base; > + val = pdc_reg_read(IRQ_i_CFG, irq); > + __assign_bit(IRQ_i_CFG_IRQ_ENABLE, &val, 0); > + pdc_reg_write(IRQ_i_CFG, irq, val); > + } This is a bit backwards. The PDC version doesn't change within the loop. But more importantly, this is a rewrite of the pdc_enable_intr() helper, only taking raw indices instead of an irq_data pointer. Surely this can be written in a better way. > } > } > > return 0; > } > > +#define QCOM_PDC_SIZE 0x30000 > + > static int qcom_pdc_init(struct device_node *node, struct device_node *parent) > { > struct irq_domain *parent_domain, *pdc_domain; > + struct resource res; > + resource_size_t res_size; nit: swapping these two lines will make things vaguely more readable. > int ret; > > - pdc_base = of_iomap(node, 0); > + /* compat with old sm8150 DT which had very small region for PDC */ > + if (of_address_to_resource(node, 0, &res)) > + return -EINVAL; > + > + res_size = max_t(resource_size_t, resource_size(&res), QCOM_PDC_SIZE); This probably deserves a warning so that DTs that do not have the correct size get fixed. Thanks, M.