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

[Dmitry Baryshkov]

On Wed, 23 Aug 2023 at 12:49, Neil Armstrong <neil.armstrong@xxxxxxxxxx> 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@xxxxxxxxxxx>
> Signed-off-by: Neil Armstrong <neil.armstrong@xxxxxxxxxx>
> ---
> 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@xxxxxxxxxx
>
> 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@xxxxxxxxxx
> ---
>  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@xxxxxxxxxx>
>


-- 
With best wishes
Dmitry