Re: [PATCH v4 1/3] gpio: xgene: Enable X-Gene standby GPIO as interrupt controller

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




On Wed, Jan 27, 2016 at 12:39 AM, Marc Zyngier <marc.zyngier@xxxxxxx> wrote:
> On 26/01/16 16:27, Quan Nguyen wrote:
>> On Tue, Jan 26, 2016 at 5:34 PM, Marc Zyngier <marc.zyngier@xxxxxxx> wrote:
>>>
>>> On 26/01/16 07:22, Quan Nguyen wrote:
>>>> 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 <yvo@xxxxxxx>
>>>> Signed-off-by: Quan Nguyen <qnguyen@xxxxxxx>
>>>> ---
>>>>  drivers/gpio/gpio-xgene-sb.c | 331 ++++++++++++++++++++++++++++++++++++-------
>>>>  1 file changed, 276 insertions(+), 55 deletions(-)
>
> [...]
>
>>>> +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?

Sorry Marc, it was me who misunderstood here.
And yes, gpiochip_lock_as_irq() should go to activate method.

>
>>>
>>>> +     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.
>>
>>>
>>>> +             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);
>>
>>>
>>>> +     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.
>>
>>>
>>>>
>>>> -     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.
>>
>>>
>>>> +}
>>>> +
>>>> +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.
>>
>>>
>>>> +
>>>> +     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.
>>
>>>
>>>> +
>>>> +     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.
>

I think I understand now. If so, calling translate here is redundant,
and hence, let me remove these code.

>>
>>>
>>>> +
>>>> +     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.
>

And here also, let me move these into translate method instead.

>>>
>>>> +
>>>> +     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.
>

What I was thinking is to have other id to match with difference
instances and these code can be use for ACPI also. Let say
"apm,xgene2-gpio-sb"
Please help correcting me if it is not right.

>>
>>>
>>>> +#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.
>>
>>>> +     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 Marc for this suggestion. I'll update DT binding document to
state that the first/lowest interrupt must be specified

Thanks Marc,
-- Quan Nguyen
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html



[Index of Archives]     [Device Tree Compilter]     [Device Tree Spec]     [Linux Driver Backports]     [Video for Linux]     [Linux USB Devel]     [Linux PCI Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Yosemite Backpacking]
  Powered by Linux