On 26/01/16 16:27, Quan Nguyen wrote:

On Tue, Jan 26, 2016 at 5:34 PM, Marc Zyngier [off-list ref] wrote:

quoted

On 26/01/16 07:22, Quan Nguyen wrote:

quoted

Enable X-Gene standby GPIO controller as interrupt controller to provide
its own resources. This avoids ambiguity where GIC interrupt resource is
use as X-Gene standby GPIO interrupt resource in user driver.

Signed-off-by: Y Vo <redacted>
Signed-off-by: Quan Nguyen <redacted>
---
 drivers/gpio/gpio-xgene-sb.c | 331 ++++++++++++++++++++++++++++++++++++-------
 1 file changed, 276 insertions(+), 55 deletions(-)

[...]

quoted

quoted

+static int xgene_gpio_sb_irq_set_type(struct irq_data *d, unsigned int type)
+{
+     struct xgene_gpio_sb *priv = irq_data_get_irq_chip_data(d);
+     int gpio = d->hwirq + IRQ_START_PIN(priv);
+     int lvl_type;
+     int ret;
+
+     switch (type & IRQ_TYPE_SENSE_MASK) {
+     case IRQ_TYPE_EDGE_RISING:
+     case IRQ_TYPE_LEVEL_HIGH:
+             lvl_type = GPIO_INT_LEVEL_H;
+             break;
+     case IRQ_TYPE_EDGE_FALLING:
+     case IRQ_TYPE_LEVEL_LOW:
+             lvl_type = GPIO_INT_LEVEL_L;
+             break;
+     default:
+             return -EINVAL;
+     }
+
+     ret = gpiochip_lock_as_irq(&priv->gc, gpio);

This has no business whatsoever in set_type. This should be done either
when the GPIO is activated as an IRQ in the domain "activate" method, or
in the "startup" method of the irqchip.

The irq pin can do high/low level as well as edge rising/falling,
while its parent(GIC) can only be high level/edge rising. Hence, there
is need to configure the irq pin to indicate its parent irq chip when
there is "high" or "low" on the pin, very much like an invert as the
code below:
xgene_gpio_set_bit(&priv->gc, priv->regs + MPA_GPIO_INT_LVL, d->hwirq,
lvl_type);

I don't get it. your "gpiochip_lock_as_irq" doesn't seem to go anywhere
the GIC, and doesn't have any knowledge of the trigger. So what is the
trick?

quoted

quoted

+     if (ret) {
+             dev_err(priv->gc.parent,
+             "Unable to configure XGene GPIO standby pin %d as IRQ\n",
+                             gpio);
+             return ret;
+     }
+
+     if ((gpio >= IRQ_START_PIN(priv)) &&
+                     (d->hwirq < NIRQ_MAX(priv))) {

How can we end-up here if your GPIO is not part that range? This should
be guaranteed by construction.

I agree, let me remove it.

quoted

quoted

+             xgene_gpio_set_bit(&priv->gc, priv->regs + MPA_GPIO_SEL_LO,
+                             gpio * 2, 1);
+             xgene_gpio_set_bit(&priv->gc, priv->regs + MPA_GPIO_INT_LVL,
+                             d->hwirq, lvl_type);
+     }
+
+     /* Propagate IRQ type setting to parent */
+     if (type & IRQ_TYPE_EDGE_BOTH)
+             return irq_chip_set_type_parent(d, IRQ_TYPE_EDGE_RISING);
+     else
+             return irq_chip_set_type_parent(d, IRQ_TYPE_LEVEL_HIGH);
+}
+
+static void xgene_gpio_sb_irq_shutdown(struct irq_data *d)
+{
+     struct xgene_gpio_sb *priv = irq_data_get_irq_chip_data(d);
+
+     gpiochip_unlock_as_irq(&priv->gc, d->hwirq + IRQ_START_PIN(priv));
+}
+
+static struct irq_chip xgene_gpio_sb_irq_chip = {
+     .name           = "sbgpio",
+     .irq_ack        = irq_chip_ack_parent,
+     .irq_eoi        = irq_chip_eoi_parent,
+     .irq_mask       = irq_chip_mask_parent,
+     .irq_unmask     = irq_chip_unmask_parent,
+     .irq_set_type   = xgene_gpio_sb_irq_set_type,
+     .irq_shutdown   = xgene_gpio_sb_irq_shutdown,
+};
+
+static int xgene_gpio_sb_to_irq(struct gpio_chip *gc, u32 gpio)
 {
      struct xgene_gpio_sb *priv = gpiochip_get_data(gc);
+     struct irq_fwspec fwspec;
+     unsigned int virq;
+
+     if ((gpio < IRQ_START_PIN(priv)) ||
+                     (gpio > NIRQ_MAX(priv) + IRQ_START_PIN(priv)))
+             return -ENXIO;
+     if (gc->parent->of_node)
+             fwspec.fwnode = of_node_to_fwnode(gc->parent->of_node);
+     else
+             fwspec.fwnode = gc->parent->fwnode;
+     fwspec.param_count = 2;
+     fwspec.param[0] = gpio - IRQ_START_PIN(priv);
+     fwspec.param[1] = IRQ_TYPE_NONE;
+     virq = irq_find_mapping(priv->irq_domain, gpio - IRQ_START_PIN(priv));
+     if (!virq)
+             virq =  irq_domain_alloc_irqs(priv->irq_domain, 1,
+                                             NUMA_NO_NODE, &fwspec);

You should not use these low-level functions directly. Use
irq_create_fwspec_mapping, which will do the right thing.

Yes, agree, the code should be much better.

Let me change:

virq = irq_find_mapping(priv->irq_domain, gpio - IRQ_START_PIN(priv));
if (!virq)
virq =  irq_domain_alloc_irqs(priv->irq_domain, 1, NUMA_NO_NODE, &fwspec);
return virq;

to:
return irq_create_fwspec_mapping(&fwspec);

quoted

quoted

+     return virq;
+}
+
+static void xgene_gpio_sb_domain_activate(struct irq_domain *d,
+             struct irq_data *irq_data)
+{
+     struct xgene_gpio_sb *priv = d->host_data;
+     u32 gpio = irq_data->hwirq + IRQ_START_PIN(priv);

-     if (priv->irq[gpio])
-             return priv->irq[gpio];
+     if ((gpio < IRQ_START_PIN(priv)) ||
+                     (gpio > NIRQ_MAX(priv) + IRQ_START_PIN(priv)))
+             return;

Again, how can this happen?

let me remove this redundant code.

quoted

quoted

-     return -ENXIO;
+     xgene_gpio_set_bit(&priv->gc, priv->regs + MPA_GPIO_SEL_LO,
+                     gpio * 2, 1);

This seems to program the interrupt to be active on a low level. Why?
Isn't that what set_type is supposed to do?

set_type currently does it, so this activate can be removed, but
deactivate() is need as it helps to convert the pin back to gpio
function.

quoted

quoted

+}
+
+static void xgene_gpio_sb_domain_deactivate(struct irq_domain *d,
+             struct irq_data *irq_data)
+{
+     struct xgene_gpio_sb *priv = d->host_data;
+     u32 gpio = irq_data->hwirq + IRQ_START_PIN(priv);

It really feels like you need a hwirq_to_gpio() accessor.

Yes. I'll add it.

quoted

quoted

+
+     if ((gpio < IRQ_START_PIN(priv)) ||
+                     (gpio > NIRQ_MAX(priv) + IRQ_START_PIN(priv)))
+             return;

Why do we need this?

Again, let me remove it.

quoted

quoted

+
+     xgene_gpio_set_bit(&priv->gc, priv->regs + MPA_GPIO_SEL_LO,
+                     gpio * 2, 0);
+}
+
+static int xgene_gpio_sb_domain_translate(struct irq_domain *d,
+             struct irq_fwspec *fwspec,
+             unsigned long *hwirq,
+             unsigned int *type)
+{
+     if (fwspec->param_count != 2)
+             return -EINVAL;
+     *hwirq = fwspec->param[0];
+     *type = fwspec->param[1];
+     return 0;
+}
+
+static int xgene_gpio_sb_domain_alloc(struct irq_domain *domain,
+                                     unsigned int virq,
+                                     unsigned int nr_irqs, void *data)
+{
+     struct irq_fwspec *fwspec = data;
+     struct irq_fwspec parent_fwspec;
+     struct xgene_gpio_sb *priv = domain->host_data;
+     irq_hw_number_t hwirq;
+     unsigned int type = IRQ_TYPE_NONE;
+     unsigned int i;
+     u32 ret;
+
+     ret = xgene_gpio_sb_domain_translate(domain, fwspec, &hwirq, &type);
+     if (ret)
+             return ret;

How can this fail here?

This could fail if wrong param_count was detected in _translate().

But you only get there if translate succeeded the first place when
called from irq_create_fwspec_mapping -> irq_domain_translate, which
happens before trying any allocation.

So I'm still stating that this cannot fail in any way.

quoted

quoted

+
+     hwirq = fwspec->param[0];
+     if ((hwirq >= NIRQ_MAX(priv)) ||
+                     (hwirq + nr_irqs > NIRQ_MAX(priv)))
+             return -EINVAL;

How can this happen?

This is for case of out of range hwirq.

Then it would be better placed in the translate method, so that we can
abort early.

quoted

quoted

+
+     for (i = 0; i < nr_irqs; i++)
+             irq_domain_set_hwirq_and_chip(domain, virq + i, hwirq + i,
+                             &xgene_gpio_sb_irq_chip, priv);
+
+     if (is_of_node(domain->parent->fwnode)) {
+             parent_fwspec.fwnode = domain->parent->fwnode;
+             parent_fwspec.param_count = 3;
+             parent_fwspec.param[0] = 0;/* SPI */
+             /* Skip SGIs and PPIs*/
+             parent_fwspec.param[1] = hwirq + START_PARENT_IRQ(priv) - 32;
+             parent_fwspec.param[2] = fwspec->param[1];
+     } else if (is_fwnode_irqchip(domain->parent->fwnode)) {
+             parent_fwspec.fwnode = domain->parent->fwnode;
+             parent_fwspec.param_count = 2;
+             parent_fwspec.param[0] = hwirq + START_PARENT_IRQ(priv);
+             parent_fwspec.param[1] = fwspec->param[1];
+     } else
+             return -EINVAL;
+
+     return irq_domain_alloc_irqs_parent(domain, virq, nr_irqs,
+                     &parent_fwspec);
+}
+
+static void xgene_gpio_sb_domain_free(struct irq_domain *domain,
+             unsigned int virq,
+             unsigned int nr_irqs)
+{
+     struct irq_data *d;
+     unsigned int i;
+
+     for (i = 0; i < nr_irqs; i++) {
+             d = irq_domain_get_irq_data(domain, virq + i);
+             irq_domain_reset_irq_data(d);
+     }
 }

+static const struct irq_domain_ops xgene_gpio_sb_domain_ops = {
+     .translate      = xgene_gpio_sb_domain_translate,
+     .alloc          = xgene_gpio_sb_domain_alloc,
+     .free           = xgene_gpio_sb_domain_free,
+     .activate       = xgene_gpio_sb_domain_activate,
+     .deactivate     = xgene_gpio_sb_domain_deactivate,
+};
+
+static const struct of_device_id xgene_gpio_sb_of_match[] = {
+     {.compatible = "apm,xgene-gpio-sb", .data = (const void *)SBGPIO_XGENE},
+     {},
+};
+MODULE_DEVICE_TABLE(of, xgene_gpio_sb_of_match);
+
+#ifdef CONFIG_ACPI
+static const struct acpi_device_id xgene_gpio_sb_acpi_match[] = {
+     {"APMC0D15", SBGPIO_XGENE},
+     {},
+};
+MODULE_DEVICE_TABLE(acpi, xgene_gpio_sb_acpi_match);
+#endif
+
 static int xgene_gpio_sb_probe(struct platform_device *pdev)
 {
      struct xgene_gpio_sb *priv;
-     u32 ret, i;
-     u32 default_lines[] = {0x08, 0x09, 0x0A, 0x0B, 0x0C, 0x0D};
+     u32 ret;
      struct resource *res;
      void __iomem *regs;
+     const struct of_device_id *of_id;
+     struct irq_domain *parent_domain = NULL;

      priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
      if (!priv)

@@ -90,6 +301,32 @@ static int xgene_gpio_sb_probe(struct platform_device *pdev)
      if (IS_ERR(regs))
              return PTR_ERR(regs);

+     priv->regs = regs;
+
+     of_id = of_match_device(xgene_gpio_sb_of_match, &pdev->dev);
+     if (of_id)
+             priv->flags = (uintptr_t)of_id->data;

Wait. Everything is hardcoded? So why do we have to deal with looking
into that structure if nothing is actually parametrized?

There will be other instances with difference number of irq pins /gpio
/start_irq_base etc.

Then it has to be described in DT right now.

quoted

quoted

+#ifdef CONFIG_ACPI
+     else {
+             const struct acpi_device_id *acpi_id;
+
+             acpi_id = acpi_match_device(xgene_gpio_sb_acpi_match,
+                             &pdev->dev);
+             if (acpi_id)
+                     priv->flags = (uintptr_t)acpi_id->driver_data;
+     }
+#endif

nit: you can write this as

        if (of_id) {
                ...
#ifdef ...
        } else {
                ...
#endif
        }


Which preserves the Linux coding style.

Thanks, let me change the code that way.

quoted

quoted

+     ret = platform_get_irq(pdev, 0);
+     if (ret > 0) {
+             priv->flags &= ~0xff;
+             priv->flags |= irq_get_irq_data(ret)->hwirq & 0xff;
+             parent_domain = irq_get_irq_data(ret)->domain;
+     }

This is rather ugly. You have the interrupt-parent property. Why don't
you look it up, and do a irq_find_matching_fwnode? Also, what guarantee
do you have that the interrupts are going to be sorted in the DT? There
is no such garantee in the documentation.

I decided to keep them because I still found difficult with ACPI
table, which does not have interrupt-parent property. This code works
with both DT and ACPI so I keep it.

Then again: what guarantees that you will have:
- the lowest interrupt listed first?
- a set contiguous interrupts?

Your DT binding doesn't specify anything of that sort, so I could write
a DT that uses interrupts 7 5 and 142, in that order. It would be legal,
and yet things would explode. So please be clear in your DT binding
about what you do support.

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...