| Message ID | 20230823-topic-sm8x50-upstream-pdc-ver-v3-1-aa7d9ab862e4@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | [v3] irqchip/qcom-pdc: add support for v3.2 HW | expand |
On Wed, 23 Aug 2023 at 12:49, Neil Armstrong <neil.armstrong@linaro.org> wrote: > > 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. > > Reviewed-by: Maulik Shah <quic_mkshah@quicinc.com> > Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org> > --- > Changes in v3: > - Simplify qcom_pdc_gic_set_type() > - Used __assign_bit in pdc_setup_pin_mapping() > - remove BIT() from IRQ_i_CFG_IRQ_ENABLE to be used with __assign_bit() > - Add Reviewed-by tag > - Link to v2: https://lore.kernel.org/r/20230822-topic-sm8x50-upstream-pdc-ver-v2-1-3035b8d388f7@linaro.org > > Changes in v2: > - Changed IRQ_ENABLE handling based on Maulik's comments > - Link to v1: https://lore.kernel.org/r/20230821-topic-sm8x50-upstream-pdc-ver-v1-1-6d7f4dd95719@linaro.org > --- > drivers/irqchip/qcom-pdc.c | 61 ++++++++++++++++++++++++++++++++++++---------- > 1 file changed, 48 insertions(+), 13 deletions(-) This patch in linux-next broke sm8150. On that platform the PDC region has size 0x400, so reading the version crashes the kernel. I'll send a patch fixing device tree, but we'd still need to handle this in a driver too. > > diff --git a/drivers/irqchip/qcom-pdc.c b/drivers/irqchip/qcom-pdc.c > index d96916cf6a41..5f60a23686c9 100644 > --- a/drivers/irqchip/qcom-pdc.c > +++ b/drivers/irqchip/qcom-pdc.c > @@ -23,9 +23,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 > + > +/* Notable PDC versions */ > +enum { > + PDC_VERSION_3_2 = 0x30200, > +}; > + > struct pdc_pin_region { > u32 pin_base; > u32 parent_base; > @@ -38,6 +51,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) > { > @@ -54,15 +68,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); > } > > @@ -143,6 +164,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); > @@ -247,7 +269,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) > @@ -278,11 +300,22 @@ 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); > + } > } > } > > @@ -300,6 +333,8 @@ static int qcom_pdc_init(struct device_node *node, struct device_node *parent) > return -ENXIO; > } > > + pdc_version = pdc_reg_read(PDC_VERSION, 0); > + > parent_domain = irq_find_host(parent); > if (!parent_domain) { > pr_err("%pOF: unable to find PDC's parent domain\n", node); > > --- > base-commit: 47d9bb711707d15b19fad18c8e2b4b027a264a3a > change-id: 20230821-topic-sm8x50-upstream-pdc-ver-114ceb45e1ee > > Best regards, > -- > Neil Armstrong <neil.armstrong@linaro.org> >
On Fri, 25 Aug 2023 19:43:34 +0100, Dmitry Baryshkov <dmitry.baryshkov@linaro.org> wrote: > > On Wed, 23 Aug 2023 at 12:49, Neil Armstrong <neil.armstrong@linaro.org> wrote: > > > > 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. > > > > Reviewed-by: Maulik Shah <quic_mkshah@quicinc.com> > > Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org> > > --- > > Changes in v3: > > - Simplify qcom_pdc_gic_set_type() > > - Used __assign_bit in pdc_setup_pin_mapping() > > - remove BIT() from IRQ_i_CFG_IRQ_ENABLE to be used with __assign_bit() > > - Add Reviewed-by tag > > - Link to v2: https://lore.kernel.org/r/20230822-topic-sm8x50-upstream-pdc-ver-v2-1-3035b8d388f7@linaro.org > > > > Changes in v2: > > - Changed IRQ_ENABLE handling based on Maulik's comments > > - Link to v1: https://lore.kernel.org/r/20230821-topic-sm8x50-upstream-pdc-ver-v1-1-6d7f4dd95719@linaro.org > > --- > > drivers/irqchip/qcom-pdc.c | 61 ++++++++++++++++++++++++++++++++++++---------- > > 1 file changed, 48 insertions(+), 13 deletions(-) > > This patch in linux-next broke sm8150. On that platform the PDC region > has size 0x400, so reading the version crashes the kernel. > I'll send a patch fixing device tree, but we'd still need to handle > this in a driver too. Thanks for reporting the breakage. Given that this has direct platform implications, I've dropped the patch from my tree until people figure out whether the proposed fix is the correct one and take the time it takes to test it. Please work with Neil to post an updated series that contains the fixed support for v3.2 and the DT churn. Thanks, M.
Hi, On 26/08/2023 12:09, Marc Zyngier wrote: > On Fri, 25 Aug 2023 19:43:34 +0100, > Dmitry Baryshkov <dmitry.baryshkov@linaro.org> wrote: >> >> On Wed, 23 Aug 2023 at 12:49, Neil Armstrong <neil.armstrong@linaro.org> wrote: >>> >>> 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. >>> >>> Reviewed-by: Maulik Shah <quic_mkshah@quicinc.com> >>> Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org> >>> --- >>> Changes in v3: >>> - Simplify qcom_pdc_gic_set_type() >>> - Used __assign_bit in pdc_setup_pin_mapping() >>> - remove BIT() from IRQ_i_CFG_IRQ_ENABLE to be used with __assign_bit() >>> - Add Reviewed-by tag >>> - Link to v2: https://lore.kernel.org/r/20230822-topic-sm8x50-upstream-pdc-ver-v2-1-3035b8d388f7@linaro.org >>> >>> Changes in v2: >>> - Changed IRQ_ENABLE handling based on Maulik's comments >>> - Link to v1: https://lore.kernel.org/r/20230821-topic-sm8x50-upstream-pdc-ver-v1-1-6d7f4dd95719@linaro.org >>> --- >>> drivers/irqchip/qcom-pdc.c | 61 ++++++++++++++++++++++++++++++++++++---------- >>> 1 file changed, 48 insertions(+), 13 deletions(-) >> >> This patch in linux-next broke sm8150. On that platform the PDC region >> has size 0x400, so reading the version crashes the kernel. >> I'll send a patch fixing device tree, but we'd still need to handle >> this in a driver too. > > Thanks for reporting the breakage. > > Given that this has direct platform implications, I've dropped the > patch from my tree until people figure out whether the proposed fix is > the correct one and take the time it takes to test it. > > Please work with Neil to post an updated series that contains the > fixed support for v3.2 and the DT churn. Thanks for dropping the change, Neil > > > Thanks, > > M. >
diff --git a/drivers/irqchip/qcom-pdc.c b/drivers/irqchip/qcom-pdc.c index d96916cf6a41..5f60a23686c9 100644 --- a/drivers/irqchip/qcom-pdc.c +++ b/drivers/irqchip/qcom-pdc.c @@ -23,9 +23,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 + +/* Notable PDC versions */ +enum { + PDC_VERSION_3_2 = 0x30200, +}; + struct pdc_pin_region { u32 pin_base; u32 parent_base; @@ -38,6 +51,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) { @@ -54,15 +68,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); } @@ -143,6 +164,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); @@ -247,7 +269,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) @@ -278,11 +300,22 @@ 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); + } } } @@ -300,6 +333,8 @@ static int qcom_pdc_init(struct device_node *node, struct device_node *parent) return -ENXIO; } + pdc_version = pdc_reg_read(PDC_VERSION, 0); + parent_domain = irq_find_host(parent); if (!parent_domain) { pr_err("%pOF: unable to find PDC's parent domain\n", node);