Date:   Fri, 04 Jan 2019 16:25:23 -0800
From:   Stephen Boyd <sboyd@...nel.org>
To:     Brian Masney <masneyb@...tation.org>, andy.gross@...aro.org,
        bjorn.andersson@...aro.org, linus.walleij@...aro.org
Cc:     marc.zyngier@....com, shawnguo@...nel.org, dianders@...omium.org,
        linux-gpio@...r.kernel.org, nicolas.dechesne@...aro.org,
        niklas.cassel@...aro.org, david.brown@...aro.org,
        robh+dt@...nel.org, mark.rutland@....com, thierry.reding@...il.com,
        linux-arm-msm@...r.kernel.org, linux-kernel@...r.kernel.org,
        devicetree@...r.kernel.org
Subject: Re: [PATCH 1/3] spmi: pmic-arb: convert to v2 irq interfaces to support hierarchical IRQ chips

Quoting Brian Masney (2018-12-29 03:47:53)
> Convert the spmi-pmic-arb IRQ code to use the version 2 IRQ interface
> in order to support hierarchical IRQ chips. Code was tested on a LG
> Nexus 5 (hammerhead) phone.

Can you add the motivation for this change here? Why should we care?

> 
> Signed-off-by: Brian Masney <masneyb@...tation.org>
> ---
>  drivers/spmi/spmi-pmic-arb.c | 91 +++++++++++++++++++++++++-----------
>  1 file changed, 64 insertions(+), 27 deletions(-)
> 
> diff --git a/drivers/spmi/spmi-pmic-arb.c b/drivers/spmi/spmi-pmic-arb.c
> index 360b8218f322..c651d19f0623 100644
> --- a/drivers/spmi/spmi-pmic-arb.c
> +++ b/drivers/spmi/spmi-pmic-arb.c
> @@ -666,7 +666,8 @@ static int qpnpint_get_irqchip_state(struct irq_data *d,
>         return 0;
>  }
>  
> -static int qpnpint_irq_request_resources(struct irq_data *d)
> +static int qpnpint_irq_domain_activate(struct irq_domain *domain,
> +                                      struct irq_data *d, bool reserve)
>  {
>         struct spmi_pmic_arb *pmic_arb = irq_data_get_irq_chip_data(d);
>         u16 periph = hwirq_to_per(d->hwirq);
> @@ -692,36 +693,37 @@ static struct irq_chip pmic_arb_irqchip = {
>         .irq_set_type   = qpnpint_irq_set_type,
>         .irq_set_wake   = qpnpint_irq_set_wake,
>         .irq_get_irqchip_state  = qpnpint_get_irqchip_state,
> -       .irq_request_resources = qpnpint_irq_request_resources,
>         .flags          = IRQCHIP_MASK_ON_SUSPEND,
>  };
>  
> -static int qpnpint_irq_domain_dt_translate(struct irq_domain *d,
> -                                          struct device_node *controller,
> -                                          const u32 *intspec,
> -                                          unsigned int intsize,
> -                                          unsigned long *out_hwirq,
> -                                          unsigned int *out_type)
> +static int qpnpint_irq_domain_translate(struct irq_domain *d,
> +                                       struct irq_fwspec *fwspec,
> +                                       unsigned long *out_hwirq,
> +                                       unsigned int *out_type)
>  {
>         struct spmi_pmic_arb *pmic_arb = d->host_data;
>         u16 apid, ppid;
>         int rc;
>  
> -       dev_dbg(&pmic_arb->spmic->dev, "intspec[0] 0x%1x intspec[1] 0x%02x intspec[2] 0x%02x\n",
> -               intspec[0], intspec[1], intspec[2]);
> +       dev_dbg(&pmic_arb->spmic->dev,
> +               "param[0] 0x%1x param[1] 0x%02x param[2] 0x%02x\n",
> +               fwspec->param[0], fwspec->param[1], fwspec->param[2]);
>  
> -       if (irq_domain_get_of_node(d) != controller)
> +       if (irq_domain_get_of_node(d) != pmic_arb->spmic->dev.of_node)
>                 return -EINVAL;
> -       if (intsize != 4)
> +       if (fwspec->param_count != 4)
>                 return -EINVAL;
> -       if (intspec[0] > 0xF || intspec[1] > 0xFF || intspec[2] > 0x7)
> +       if (fwspec->param[0] > 0xF || fwspec->param[1] > 0xFF ||
> +           fwspec->param[2] > 0x7)
>                 return -EINVAL;
>  
> -       ppid = intspec[0] << 8 | intspec[1];
> +       ppid = fwspec->param[0] << 8 | fwspec->param[1];
>         rc = pmic_arb->ver_ops->ppid_to_apid(pmic_arb, ppid);
>         if (rc < 0) {
> -               dev_err(&pmic_arb->spmic->dev, "failed to xlate sid = %#x, periph = %#x, irq = %u rc = %d\n",
> -               intspec[0], intspec[1], intspec[2], rc);
> +               dev_err(&pmic_arb->spmic->dev,
> +                       "failed to xlate sid = %#x, periph = %#x, irq = %u rc = %d\n",
> +                       fwspec->param[0], fwspec->param[1], fwspec->param[2],

Why not assign a u32 *intspec to fwspec->param and then not change many
lines in this function? Reducing the diff is useful to avoid glossing
over some mistake in variable renames.

> +                       rc);
>                 return rc;
>         }
>  
> @@ -732,25 +734,58 @@ static int qpnpint_irq_domain_dt_translate(struct irq_domain *d,
>         if (apid < pmic_arb->min_apid)
>                 pmic_arb->min_apid = apid;
>  
> -       *out_hwirq = spec_to_hwirq(intspec[0], intspec[1], intspec[2], apid);
> -       *out_type  = intspec[3] & IRQ_TYPE_SENSE_MASK;
> +       *out_hwirq = spec_to_hwirq(fwspec->param[0], fwspec->param[1],
> +                                  fwspec->param[2], apid);
> +       *out_type  = fwspec->param[3] & IRQ_TYPE_SENSE_MASK;
>  
>         dev_dbg(&pmic_arb->spmic->dev, "out_hwirq = %lu\n", *out_hwirq);
>  
>         return 0;
>  }
>  
> -static int qpnpint_irq_domain_map(struct irq_domain *d,
> -                                 unsigned int virq,
> -                                 irq_hw_number_t hwirq)
> +
> +static void qpnpint_irq_domain_map(struct spmi_pmic_arb *pmic_arb,
> +                                  struct irq_domain *domain, unsigned int virq,
> +                                  irq_hw_number_t hwirq)
>  {
> -       struct spmi_pmic_arb *pmic_arb = d->host_data;
> +       unsigned int old_virq;
>  
>         dev_dbg(&pmic_arb->spmic->dev, "virq = %u, hwirq = %lu\n", virq, hwirq);
>  
> -       irq_set_chip_and_handler(virq, &pmic_arb_irqchip, handle_level_irq);
> -       irq_set_chip_data(virq, d->host_data);
> -       irq_set_noprobe(virq);
> +       /*
> +        * Check to see if the hwirq is already associated with another virq on
> +        * this IRQ domain. If so, then disassociate it before associating the
> +        * hwirq with the new virq. IRQs are all initially setup without an IRQ
> +        * hierarchy when this driver is probed and when mfd/qcom-spmi-pmic.c is
> +        * probed. Later in the boot process, an IRQ hierarchy is requested by
> +        * pinctrl-spmi-gpio.c, and the same hwirq is now associated with a new
> +        * virq.

Is that because we count the irqs in the pmic gpio driver? It looks like
we should remove that code from the driver then and use the compatible
string to figure out how many gpios are present. It will make this
comment and disassociate code unnecessary, but unfortunately it will
mean we have to add all the compatible strings to the pmic gpio driver
just so we know how many gpios are there.

> +        */
> +       old_virq = irq_find_mapping(domain, hwirq);
> +       if (old_virq)
> +               irq_domain_disassociate(domain, old_virq);
> +
> +       irq_domain_set_info(domain, virq, hwirq, &pmic_arb_irqchip, pmic_arb,
> +                           handle_level_irq, NULL, NULL);
> +}
> +
> +static int qpnpint_irq_domain_alloc(struct irq_domain *domain,
> +                                   unsigned int virq, unsigned int nr_irqs,
> +                                   void *data)
> +{
> +       struct spmi_pmic_arb *pmic_arb = domain->host_data;
> +       struct irq_fwspec *fwspec = data;
> +       irq_hw_number_t hwirq;
> +       unsigned int type;
> +       int ret, i;
> +
> +       ret = qpnpint_irq_domain_translate(domain, fwspec, &hwirq, &type);
> +       if (ret)
> +               return ret;
> +
> +       for (i = 0; i < nr_irqs; i++)
> +               qpnpint_irq_domain_map(pmic_arb, domain, virq + i, hwirq + i);
> +
>         return 0;
>  }
>  

Otherwise patch looks good to me. Thanks for working on this. Are you
going to convert the ssbi master and gpio chip too?

Powered by blists - more mailing lists