On 24/07/18 16:02, Parthiban Nallathambi wrote:
> Actions Semi Owl family SoC's S500, S700 and S900 provides support
> for 3 external interrupt controllers through SIRQ pins.
>
> Each line can be independently configured as interrupt or wake-up source,
> and triggers either on rising, falling or both edges. Each line can also
> be masked independently.
>
> Signed-off-by: Parthiban Nallathambi <pn@xxxxxxx>
> Signed-off-by: Saravanan Sekar <sravanhome@xxxxxxxxx>
> ---
> drivers/irqchip/Makefile | 1 +
> drivers/irqchip/irq-owl-sirq.c | 275 +++++++++++++++++++++++++++++++++++++++++
> 2 files changed, 276 insertions(+)
> create mode 100644 drivers/irqchip/irq-owl-sirq.c
>
> diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
> index 15f268f646bf..072c4409e7c4 100644
> --- a/drivers/irqchip/Makefile
> +++ b/drivers/irqchip/Makefile
> @@ -7,6 +7,7 @@ obj-$(CONFIG_ATH79) += irq-ath79-misc.o
> obj-$(CONFIG_ARCH_BCM2835) += irq-bcm2835.o
> obj-$(CONFIG_ARCH_BCM2835) += irq-bcm2836.o
> obj-$(CONFIG_ARCH_EXYNOS) += exynos-combiner.o
> +obj-$(CONFIG_ARCH_ACTIONS) += irq-owl-sirq.o
> obj-$(CONFIG_FARADAY_FTINTC010) += irq-ftintc010.o
> obj-$(CONFIG_ARCH_HIP04) += irq-hip04.o
> obj-$(CONFIG_ARCH_LPC32XX) += irq-lpc32xx.o
> diff --git a/drivers/irqchip/irq-owl-sirq.c b/drivers/irqchip/irq-owl-sirq.c
> new file mode 100644
> index 000000000000..8605da99d77d
> --- /dev/null
> +++ b/drivers/irqchip/irq-owl-sirq.c
> @@ -0,0 +1,275 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + *
> + * Actions Semi Owl SoCs SIRQ interrupt controller driver
> + *
> + * Copyright (C) 2014 Actions Semi Inc.
> + * David Liu <liuwei@xxxxxxxxxxxxxxxx>
> + *
> + * Author: Parthiban Nallathambi <pn@xxxxxxx>
> + * Author: Saravanan Sekar <sravanhome@xxxxxxxxx>
> + */
> +
> +#include <linux/io.h>
> +#include <linux/irq.h>
> +#include <linux/of_irq.h>
> +#include <linux/irqchip.h>
> +#include <linux/spinlock.h>
> +#include <linux/interrupt.h>
> +#include <linux/irqdomain.h>
> +#include <linux/of_address.h>
> +#include <linux/irqchip/chained_irq.h>
> +
> +#define OWL_MAX_NR_SIRQS 3
> +
> +/* INTC_EXTCTL register offset for S900 */
> +#define S900_INTC_EXTCTL0 0x200
> +#define S900_INTC_EXTCTL1 0x528
> +#define S900_INTC_EXTCTL2 0x52C
> +
> +/* INTC_EXTCTL register offset for S700 */
> +#define S700_INTC_EXTCTL 0x200
> +
> +#define INTC_EXTCTL_PENDING BIT(0)
> +#define INTC_EXTCTL_CLK_SEL BIT(4)
> +#define INTC_EXTCTL_EN BIT(5)
> +#define INTC_EXTCTL_TYPE_MASK GENMASK(6, 7)
> +#define INTC_EXTCTL_TYPE_HIGH 0
> +#define INTC_EXTCTL_TYPE_LOW BIT(6)
> +#define INTC_EXTCTL_TYPE_RISING BIT(7)
> +#define INTC_EXTCTL_TYPE_FALLING (BIT(6) | BIT(7))
> +
> +struct owl_sirq_info {
> + void __iomem *base;
> + struct irq_domain *irq_domain;
This is completely global, right? Why do you have this in a
per-interrupt structure?
> + unsigned long reg;
> + unsigned long hwirq;
> + unsigned int virq;
> + unsigned int parent_irq;
> + bool share_reg;
> + spinlock_t lock;
This should be a raw_spinlock_t for the sake of making RT's life easier.
> +};
> +
> +/* s900 has INTC_EXTCTL individual register to handle each line */
> +static struct owl_sirq_info s900_sirq_info[OWL_MAX_NR_SIRQS] = {
> + { .reg = S900_INTC_EXTCTL0, .share_reg = false },
> + { .reg = S900_INTC_EXTCTL1, .share_reg = false },
> + { .reg = S900_INTC_EXTCTL2, .share_reg = false },
> +};
> +
> +/* s500 and s700 shares the 32 bit (24 usable) register for each line */
> +static struct owl_sirq_info s700_sirq_info[OWL_MAX_NR_SIRQS] = {
> + { .reg = S700_INTC_EXTCTL, .share_reg = true },
> + { .reg = S700_INTC_EXTCTL, .share_reg = true },
> + { .reg = S700_INTC_EXTCTL, .share_reg = true },
> +};
> +
> +static unsigned int sirq_read_extctl(struct owl_sirq_info *sirq)
> +{
> + unsigned int val;
> +
> + val = readl_relaxed(sirq->base + sirq->reg);
> + if (sirq->share_reg)
Is there a system that can have both "shared" and "non-shared"
controllers? If that's not the case (which I strongly suspect), why
isn't that a global property, a static key, or even a different set of
operations altogether?
> + val = (val >> (2 - sirq->hwirq) * 8) & 0xff;
> +
> + return val;
> +}
> +
> +static void sirq_write_extctl(struct owl_sirq_info *sirq, unsigned int extctl)
> +{
> + unsigned int val;
> +
> + if (sirq->share_reg) {
> + val = readl_relaxed(sirq->base + sirq->reg);
> + val &= ~(0xff << (2 - sirq->hwirq) * 8);
> + extctl &= 0xff;
> + extctl = (extctl << (2 - sirq->hwirq) * 8) | val;
> + }
> +
> + writel_relaxed(extctl, sirq->base + sirq->reg);
> +}
> +
> +static void owl_sirq_ack(struct irq_data *d)
> +{
> + struct owl_sirq_info *sirq = irq_data_get_irq_chip_data(d);
> + unsigned int extctl;
> + unsigned long flags;
> +
> + spin_lock_irqsave(&sirq->lock, flags);
> +
> + extctl = sirq_read_extctl(sirq);
> + extctl |= INTC_EXTCTL_PENDING;
> + sirq_write_extctl(sirq, extctl);
> +
> + spin_unlock_irqrestore(&sirq->lock, flags);
> +}
> +
> +static void owl_sirq_mask(struct irq_data *d)
> +{
> + struct owl_sirq_info *sirq = irq_data_get_irq_chip_data(d);
> + unsigned int extctl;
> + unsigned long flags;
> +
> + spin_lock_irqsave(&sirq->lock, flags);
> +
> + extctl = sirq_read_extctl(sirq);
> + extctl &= ~(INTC_EXTCTL_EN);
> + sirq_write_extctl(sirq, extctl);
> +
> + spin_unlock_irqrestore(&sirq->lock, flags);
> +}
> +
> +static void owl_sirq_unmask(struct irq_data *d)
> +{
> + struct owl_sirq_info *sirq = irq_data_get_irq_chip_data(d);
> + unsigned int extctl;
> + unsigned long flags;
> +
> + spin_lock_irqsave(&sirq->lock, flags);
> +
> + /* we don't hold the irq pending generated before irq enabled */
What do you mean here?
> + extctl = sirq_read_extctl(sirq);
> + extctl |= INTC_EXTCTL_EN;
> + sirq_write_extctl(sirq, extctl);
> +
> + spin_unlock_irqrestore(&sirq->lock, flags);
> +}
> +
> +/* PAD_PULLCTL needs to be defined in pinctrl */
> +static int owl_sirq_set_type(struct irq_data *d, unsigned int flow_type)
> +{
> + struct owl_sirq_info *sirq = irq_data_get_irq_chip_data(d);
> + unsigned int extctl, type;
> + unsigned long flags;
> +
> + switch (flow_type) {
> + case IRQF_TRIGGER_LOW:
> + type = INTC_EXTCTL_TYPE_LOW;
> + break;
> + case IRQF_TRIGGER_HIGH:
> + type = INTC_EXTCTL_TYPE_HIGH;
> + break;
> + case IRQF_TRIGGER_FALLING:
> + type = INTC_EXTCTL_TYPE_FALLING;
> + break;
> + case IRQF_TRIGGER_RISING:
> + type = INTC_EXTCTL_TYPE_RISING;
You advertise being able to trigger on both edges in your commit
description, but this doesn't seem to be the case here.
> + break;
> + default:
> + return -EINVAL;
> + }
> +
> + spin_lock_irqsave(&sirq->lock, flags);
> +
> + extctl = sirq_read_extctl(sirq);
> + extctl &= ~(INTC_EXTCTL_PENDING | INTC_EXTCTL_TYPE_MASK);
> + extctl |= type;
> + sirq_write_extctl(sirq, extctl);
> +
> + spin_unlock_irqrestore(&sirq->lock, flags);
And what about the parent IRQ? is the signalling always level?
> +
> + return 0;
> +}
> +
> +static void owl_sirq_handler(struct irq_desc *desc)
> +{
> + struct owl_sirq_info *sirq = irq_desc_get_handler_data(desc);
> + struct irq_chip *chip = irq_desc_get_chip(desc);
> + unsigned int extctl;
> +
> + chained_irq_enter(chip, desc);
> +
> + extctl = sirq_read_extctl(sirq);
> + if (extctl & INTC_EXTCTL_PENDING)
> + generic_handle_irq(sirq->virq);
> +
> + chained_irq_exit(chip, desc);
> +}
> +
> +static struct irq_chip owl_irq_chip = {
> + .name = "owl-sirq",
> + .irq_ack = owl_sirq_ack,
> + .irq_mask = owl_sirq_mask,
> + .irq_unmask = owl_sirq_unmask,
> + .irq_set_type = owl_sirq_set_type,
Where is the wake-up handling that you advertised above?
> +};
> +
> +static int __init owl_sirq_init(struct owl_sirq_info *sirq_info, int nr_sirq,
> + struct device_node *np)
> +{
> + struct owl_sirq_info *sirq;
> + void __iomem *base;
> + struct irq_domain *irq_domain;
> + int i, irq_base;
> + unsigned int extctl;
> + u8 sample_clk[OWL_MAX_NR_SIRQS];
> +
> + base = of_iomap(np, 0);
> + if (!base)
> + return -ENOMEM;
> +
> + irq_base = irq_alloc_descs(-1, 32, nr_sirq, 0);
No. Descriptors are supposed to be allocated on demand by core code, and
not by irqchip drivers.
> + if (irq_base < 0) {
> + pr_err("sirq: failed to allocate IRQ numbers\n");
> + goto out_unmap;
> + }
> +
> + irq_domain = irq_domain_add_legacy(np, nr_sirq, irq_base, 0,
> + &irq_domain_simple_ops, NULL);
Legacy? What legacy? A modern DT platform has no need for this, so
please stick to a linear domain (though see below).
> + if (!irq_domain) {
> + pr_err("sirq: irq domain init failed\n");
> + goto out_free_desc;
> + }
> +
> + memset(sample_clk, 0, sizeof(sample_clk));
> + of_property_read_u8_array(np, "sampling-rate-24m", sample_clk,
> + nr_sirq);
I find this most bizarre. Why is the sampling rate configurable? How do
you even decide which sample rate you want? Frankly, I'd rather see
24MHz being the default (because 32KHz feels incredibly low).
> +
> + for (i = 0; i < nr_sirq; i++) {
> + sirq = &sirq_info[i];
> +
> + sirq->base = base;
> + sirq->irq_domain = irq_domain;
> + sirq->hwirq = i;
> + sirq->virq = irq_base + i;
> +
> + sirq->parent_irq = irq_of_parse_and_map(np, i);
> + irq_set_handler_data(sirq->parent_irq, sirq);
> + irq_set_chained_handler_and_data(sirq->parent_irq,
> + owl_sirq_handler, sirq);
> +
> + irq_set_chip_and_handler(sirq->virq, &owl_irq_chip,
> + handle_level_irq);
> + irq_set_chip_data(sirq->virq, sirq);
> +
> + if (sample_clk[i]) {
> + extctl = sirq_read_extctl(sirq);
> + extctl |= INTC_EXTCTL_CLK_SEL;
> + sirq_write_extctl(sirq, extctl);
> + }
> + spin_lock_init(&sirq->lock);
> + }
I have the feeling you may have the wrong end of the stick here. The way
I understand your system, you have 3 SPIs that are brought out of the
SoC, with a tiny bit of glue logic that allows the interrupt polarity to
be tweaked.
If I understood it right, then this should be represented as a
hierarchical irqchip, such as drivers/irqchip/irq-mtk-sysirq.c which
does the same thing. The whole ack/mask dance feels completely
pointless, as we already have the same controls at the GIC level.
> +
> + return 0;
> +
> +out_free_desc:
> + irq_free_descs(irq_base, nr_sirq);
> +out_unmap:
> + iounmap(base);
> +
> + return -EINVAL;
> +}
> +
> +static int __init s700_sirq_of_init(struct device_node *np,
> + struct device_node *parent)
> +{
> + return owl_sirq_init(s700_sirq_info, ARRAY_SIZE(s700_sirq_info), np);
> +}
> +IRQCHIP_DECLARE(s700_sirq, "actions,s700-sirq", s700_sirq_of_init);
> +
> +static int __init s900_sirq_of_init(struct device_node *np,
> + struct device_node *parent)
> +{
> + return owl_sirq_init(s900_sirq_info, ARRAY_SIZE(s900_sirq_info), np);
> +}
> +IRQCHIP_DECLARE(s900_sirq, "actions,s900-sirq", s900_sirq_of_init);
>
Thanks,
M.
--
Jazz is not dead. It just smells funny...
--
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
