Re: [PATCH v2 2/3] drivers/irqchip: Add Actions external interrupts support
From: Parthiban Nallathambi
Date: Tue Nov 06 2018 - 13:07:52 EST
Hello Marc,
Ping on this patch for feedback.
On 9/20/18 11:42 AM, Parthiban Nallathambi wrote:
Hello Marc,
Ping on this patch for feedback.
On 08/26/2018 05:20 PM, Parthiban Nallathambi wrote:
Hello Marc,
Thanks for your feedback.
On 8/13/18 1:46 PM, Marc Zyngier wrote:
On 12/08/18 13:22, 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 and triggers
on either of the edges (raising or falling) or either of the levels
(high or low) . 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 | 305
+++++++++++++++++++++++++++++++++++++++++
 2 files changed, 306 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..b69301388300
--- /dev/null
+++ b/drivers/irqchip/irq-owl-sirq.c
@@ -0,0 +1,305 @@
+// 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/interrupt.h>
+#include <linux/irqchip.h>
+#include <linux/of_irq.h>
+#include <linux/of_address.h>
+
+#include <dt-bindings/interrupt-controller/arm-gic.h>
+
+#define INTC_GIC_INTERRUPT_PINÂÂÂÂÂÂÂ 13
Why isn't that coming from the DT?
DT numbering is taken irqchip local, by which hwirq is directly used to
calculate the offset into register when it is shared. Even if this is
directly from DT, I need the value to offset into the register. So
maintianed
inside the driver.
Should it make sense to move it to DT and use another macro (different
name)
for offsetting?
+#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))
+
+#define get_sirq_offset(x)ÂÂÂ chip_data->sirq[x].offset
+
+/* Per SIRQ data */
+struct owl_sirq {
+ÂÂÂ u16 offset;
+ÂÂÂ /* software is responsible to clear interrupt pending bit when
+ÂÂÂÂ * type is edge triggered. This value is for per SIRQ line.
+ÂÂÂÂ */
Please follow the normal multi-line comment style:
/*
 * This is a comment, starting with a capital letter and ending with
 * a full stop.
 */
Sure, thanks.
+ÂÂÂ bool type_edge;
+};
+
+struct owl_sirq_chip_data {
+ÂÂÂ void __iomem *base;
+ÂÂÂ raw_spinlock_t lock;
+ÂÂÂ /* some SoC's share the register for all SIRQ lines, so maintain
+ÂÂÂÂ * register is shared or not here. This value is from DT.
+ÂÂÂÂ */
+ÂÂÂ bool shared_reg;
+ÂÂÂ struct owl_sirq *sirq;
Given that this driver handles at most 3 interrupts, do we need the
overhead of a pointer and an additional allocation, while we could store
all of this data in the space taken by the pointer itself?
Something like:
ÂÂÂÂu16 offset[3];
ÂÂÂÂu8Â trigger; // Bit mask indicating edge-triggered interrupts
and we're done.
Sure, I will modify with one allocation.
+};
+
+static struct owl_sirq_chip_data *sirq_data;
+
+static unsigned int sirq_read_extctl(struct irq_data *data)
Why isn't this taking a struct owl_sirq_chip_data as a parameter instead
of always passing irq_data?
Also, this should return a well defined size, which "unsigned int"
isn't. Make that u32.
Sure, will adapt this.
+{
+ÂÂÂ struct owl_sirq_chip_data *chip_data = data->chip_data;
+ÂÂÂ unsigned int val;
u32;
Sure.
+
+ÂÂÂ val = readl_relaxed(chip_data->base +
get_sirq_offset(data->hwirq));
+ÂÂÂ if (chip_data->shared_reg)
+ÂÂÂÂÂÂÂ val = (val >> (2 - data->hwirq) * 8) & 0xff;
+
+ÂÂÂ return val;
+}
+
+static void sirq_write_extctl(struct irq_data *data, unsigned int
extctl)
Same comments.
Sure.
+{
+ÂÂÂ struct owl_sirq_chip_data *chip_data = data->chip_data;
+ÂÂÂ unsigned int val;
u32;
Sure.
+
+ÂÂÂ if (chip_data->shared_reg) {
+ÂÂÂÂÂÂÂ val = readl_relaxed(chip_data->base +
+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ get_sirq_offset(data->hwirq));
Single line, please.
Sure.
+ÂÂÂÂÂÂÂ val &= ~(0xff << (2 - data->hwirq) * 8);
+ÂÂÂÂÂÂÂ extctl &= 0xff;
+ÂÂÂÂÂÂÂ extctl = (extctl << (2 - data->hwirq) * 8) | val;
+ÂÂÂ }
+
+ÂÂÂ writel_relaxed(extctl, chip_data->base +
+ÂÂÂÂÂÂÂÂÂÂÂ get_sirq_offset(data->hwirq));
Single line.
Sure.
+}
+
+static void owl_sirq_ack(struct irq_data *data)
+{
+ÂÂÂ struct owl_sirq_chip_data *chip_data = data->chip_data;
+ÂÂÂ unsigned int extctl;
+ÂÂÂ unsigned long flags;
+
+ÂÂÂ /* software must clear external interrupt pending, when
interrupt type
+ÂÂÂÂ * is edge triggered, so we need per SIRQ based clearing.
+ÂÂÂÂ */
+ÂÂÂ if (chip_data->sirq[data->hwirq].type_edge) {
+ÂÂÂÂÂÂÂ raw_spin_lock_irqsave(&chip_data->lock, flags);
+
+ÂÂÂÂÂÂÂ extctl = sirq_read_extctl(data);
+ÂÂÂÂÂÂÂ extctl |= INTC_EXTCTL_PENDING;
+ÂÂÂÂÂÂÂ sirq_write_extctl(data, extctl);
+
+ÂÂÂÂÂÂÂ raw_spin_unlock_irqrestore(&chip_data->lock, flags);
It would make a lot more sense if the lock was taken inside the accessor
so that the rest of the driver doesn't have to deal with it. Something
along of the line of:
static void sirq_clear_set_extctl(struct owl_sirq_chip_data *d,
ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ u32 clear, u32 set)
{
ÂÂÂÂunsigned long flags;
ÂÂÂÂu32 val;
ÂÂÂÂraw_spin_lock_irqsave(&d->lock, flags);
ÂÂÂÂval = sirq_read_extctl(d);
ÂÂÂÂval &= ~clear;
ÂÂÂÂval |= set;
ÂÂÂÂsirq_write_extctl(d, val);
ÂÂÂÂraw_spin_unlock_irqrestore(&d->lock, flags)
}
And use that throughout the driver.
Thanks for sharing the function with lock, will update it.
+ÂÂÂ }
+ÂÂÂ irq_chip_ack_parent(data);
That being said, I'm terribly sceptical about this whole function. At
the end of the day, the flow handler used by the GIC is
handle_fasteoi_irq, which doesn't call the ack callback at all. So how
does this work?
That's my mistake. I will move this function for ".irq_eoi". Will that
be fine?
In short, all the devices/interrupt controller connected to sirq lines
are level
triggered in my board. So, I couldn't test this part last time.
+}
+
+static void owl_sirq_mask(struct irq_data *data)
+{
+ÂÂÂ struct owl_sirq_chip_data *chip_data = data->chip_data;
+ÂÂÂ unsigned int extctl;
+ÂÂÂ unsigned long flags;
+
+ÂÂÂ raw_spin_lock_irqsave(&chip_data->lock, flags);
+
+ÂÂÂ extctl = sirq_read_extctl(data);
+ÂÂÂ extctl &= ~(INTC_EXTCTL_EN);
+ÂÂÂ sirq_write_extctl(data, extctl);
+
+ÂÂÂ raw_spin_unlock_irqrestore(&chip_data->lock, flags);
+ÂÂÂ irq_chip_mask_parent(data);
+}
+
+static void owl_sirq_unmask(struct irq_data *data)
+{
+ÂÂÂ struct owl_sirq_chip_data *chip_data = data->chip_data;
+ÂÂÂ unsigned int extctl;
+ÂÂÂ unsigned long flags;
+
+ÂÂÂ raw_spin_lock_irqsave(&chip_data->lock, flags);
+
+ÂÂÂ extctl = sirq_read_extctl(data);
+ÂÂÂ extctl |= INTC_EXTCTL_EN;
+ÂÂÂ sirq_write_extctl(data, extctl);
+
+ÂÂÂ raw_spin_unlock_irqrestore(&chip_data->lock, flags);
+ÂÂÂ irq_chip_unmask_parent(data);
+}
+
+/* PAD_PULLCTL needs to be defined in pinctrl */
+static int owl_sirq_set_type(struct irq_data *data, unsigned int
flow_type)
+{
+ÂÂÂ struct owl_sirq_chip_data *chip_data = data->chip_data;
+ÂÂÂ 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;
+ÂÂÂÂÂÂÂ chip_data->sirq[data->hwirq].type_edge = true;
+ÂÂÂÂÂÂÂ break;
+ÂÂÂ case IRQF_TRIGGER_RISING:
+ÂÂÂÂÂÂÂ type = INTC_EXTCTL_TYPE_RISING;
+ÂÂÂÂÂÂÂ chip_data->sirq[data->hwirq].type_edge = true;
+ÂÂÂÂÂÂÂ break;
So let's say I configure an interrupt as edge, then switch it to level.
The edge setting remains and bad things will happen.
Ok, I will update the value to false for edge cases.
+ÂÂÂ default:
+ return -EINVAL;
+ÂÂÂ }
+
+ÂÂÂ raw_spin_lock_irqsave(&chip_data->lock, flags);
+
+ÂÂÂ extctl = sirq_read_extctl(data);
+ÂÂÂ extctl &= ~INTC_EXTCTL_TYPE_MASK;
+ÂÂÂ extctl |= type;
+ÂÂÂ sirq_write_extctl(data, extctl);
+
+ÂÂÂ raw_spin_unlock_irqrestore(&chip_data->lock, flags);
+ÂÂÂ data = data->parent_data;
+ÂÂÂ return irq_chip_set_type_parent(data, flow_type);
+}
+
+static struct irq_chip owl_sirq_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,
+ÂÂÂ .irq_eoiÂÂÂ = irq_chip_eoi_parent,
+ÂÂÂ .irq_retriggerÂÂÂ = irq_chip_retrigger_hierarchy,
+};
+
+static int owl_sirq_domain_alloc(struct irq_domain *domain,
unsigned int virq,
+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ unsigned int nr_irqs, void *arg)
+{
+ÂÂÂ struct irq_fwspec *fwspec = arg;
+ÂÂÂ struct irq_fwspec parent_fwspec = {
+ÂÂÂÂÂÂÂ .param_countÂÂÂ = 3,
+ÂÂÂÂÂÂÂ .param[0]ÂÂÂ = GIC_SPI,
+ÂÂÂÂÂÂÂ .param[1]ÂÂÂ = fwspec->param[0] + INTC_GIC_INTERRUPT_PIN,
+ÂÂÂÂÂÂÂ .param[2]ÂÂÂ = fwspec->param[1],
param[2] is supposed to be the trigger configuration. Your driver
supports LEVEL_LOW and EDGE_FALLING, which the GIC cannot handle. And
yet you're passing it directly.
That's my mistake. I will translate and restrict LEVEL_HIGH and
EDGE_RISING
for GIC here. Thanks.
+ÂÂÂÂÂÂÂ .fwnodeÂÂÂÂÂÂÂ = domain->parent->fwnode,
+ÂÂÂ };
+
+ÂÂÂ if (WARN_ON(nr_irqs != 1))
+ÂÂÂÂÂÂÂ return -EINVAL;
+
+ÂÂÂ irq_domain_set_hwirq_and_chip(domain, virq, fwspec->param[0],
+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ &owl_sirq_chip,
+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ domain->host_data);
+
+ÂÂÂ return irq_domain_alloc_irqs_parent(domain, virq, nr_irqs,
+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ &parent_fwspec);
+}
+
+static const struct irq_domain_ops sirq_domain_ops = {
+ÂÂÂ .allocÂÂÂ = owl_sirq_domain_alloc,
+ÂÂÂ .freeÂÂÂ = irq_domain_free_irqs_common,
No translation method? Again, how does this work?
Missed this part, I will update this next version.
+};
+
+static void owl_sirq_clk_init(int offset, int hwirq)
+{
+ÂÂÂ unsigned int val;
+
+ÂÂÂ /* register default clock is 32Khz, change to 24Mhz only when
defined */
+ÂÂÂ val = readl_relaxed(sirq_data->base + offset);
+ÂÂÂ if (sirq_data->shared_reg)
+ÂÂÂÂÂÂÂ val |= INTC_EXTCTL_CLK_SEL << (2 - hwirq) * 8;
+ÂÂÂ else
+ÂÂÂÂÂÂÂ val |= INTC_EXTCTL_CLK_SEL;
+
+ÂÂÂ writel_relaxed(val, sirq_data->base + offset);
+}
I've asked questions about this in the first review, and you didn't
answer. Why is it even configurable? How do you choose the sample rate?
What's the drawback of always setting it one way or the other?
The provision for selecting sampling rate here seems meant for power
management, which I wasn't aware of. So this configuration doesn't need
to come from DT.
Possibly this needs to be implemented as "syscore_ops" suspend and resume
calls. Should I register this as "register_syscore_ops" or leaving 32MHz
is fine?
+
+static int __init owl_sirq_of_init(struct device_node *node,
+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ struct device_node *parent)
+{
+ÂÂÂ struct irq_domain *domain, *domain_parent;
+ÂÂÂ int ret = 0, i, sirq_cnt = 0;
+ÂÂÂ struct owl_sirq_chip_data *chip_data;
+
+ÂÂÂ sirq_cnt = of_property_count_u32_elems(node,
"actions,sirq-offset");
+ÂÂÂ if (sirq_cnt <= 0) {
+ÂÂÂÂÂÂÂ pr_err("owl_sirq: register offset not specified\n");
+ÂÂÂÂÂÂÂ return -EINVAL;
+ÂÂÂ }
+
+ÂÂÂ chip_data = kzalloc(sizeof(*chip_data), GFP_KERNEL);
+ÂÂÂ if (!chip_data)
+ÂÂÂÂÂÂÂ return -ENOMEM;
+ÂÂÂ sirq_data = chip_data;
+
+ÂÂÂ chip_data->sirq = kcalloc(sirq_cnt, sizeof(*chip_data->sirq),
+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ GFP_KERNEL);
+ÂÂÂ if (!chip_data->sirq)
+ÂÂÂÂÂÂÂ goto out_free;
+
+ÂÂÂ raw_spin_lock_init(&chip_data->lock);
+ÂÂÂ chip_data->base = of_iomap(node, 0);
+ÂÂÂ if (!chip_data->base) {
+ÂÂÂÂÂÂÂ pr_err("owl_sirq: unable to map sirq register\n");
+ÂÂÂÂÂÂÂ ret = -ENXIO;
+ÂÂÂÂÂÂÂ goto out_free;
+ÂÂÂ }
+
+ÂÂÂ chip_data->shared_reg = of_property_read_bool(node,
+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ "actions,sirq-shared-reg");
+ÂÂÂ for (i = 0; i < sirq_cnt; i++) {
+ÂÂÂÂÂÂÂ u32 value;
+
+ÂÂÂÂÂÂÂ ret = of_property_read_u32_index(node, "actions,sirq-offset",
+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ i, &value);
+ÂÂÂÂÂÂÂ if (ret)
+ÂÂÂÂÂÂÂÂÂÂÂ goto out_unmap;
+
+ÂÂÂÂÂÂÂ get_sirq_offset(i) = (u16)value;
+
+ÂÂÂÂÂÂÂ ret = of_property_read_u32_index(node, "actions,sirq-clk-sel",
+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ i, &value);
+ÂÂÂÂÂÂÂ if (ret || !value)
+ÂÂÂÂÂÂÂÂÂÂÂ continue;
+
+ÂÂÂÂÂÂÂ /* external interrupt controller can be either connect to
32Khz/
+ÂÂÂÂÂÂÂÂ * 24Mhz external/internal clock. This shall be configured for
+ÂÂÂÂÂÂÂÂ * per SIRQ line. It can be defined from DT, failing
defaults to
+ÂÂÂÂÂÂÂÂ * 24Mhz clock.
+ÂÂÂÂÂÂÂÂ */
+ÂÂÂÂÂÂÂ owl_sirq_clk_init(get_sirq_offset(i), i);
+ÂÂÂ }
+
+ÂÂÂ domain_parent = irq_find_host(parent);
+ÂÂÂ if (!domain_parent) {
+ÂÂÂÂÂÂÂ pr_err("owl_sirq: interrupt-parent not found\n");
+ÂÂÂÂÂÂÂ goto out_unmap;
+ÂÂÂ }
+
+ÂÂÂ domain = irq_domain_add_hierarchy(domain_parent, 0,
+ÂÂÂÂÂÂÂÂÂÂÂ sirq_cnt, node,
+ÂÂÂÂÂÂÂÂÂÂÂ &sirq_domain_ops, chip_data);
+ÂÂÂ if (!domain) {
+ÂÂÂÂÂÂÂ ret = -ENOMEM;
+ÂÂÂÂÂÂÂ goto out_unmap;
+ÂÂÂ }
+
+ÂÂÂ return 0;
+
+out_unmap:
+ÂÂÂ iounmap(chip_data->base);
+out_free:
+ÂÂÂ kfree(chip_data);
+ÂÂÂ kfree(chip_data->sirq);
+ÂÂÂ return ret;
+}
+
+IRQCHIP_DECLARE(owl_sirq, "actions,owl-sirq", owl_sirq_of_init);
As it stands, this driver is nowhere near ready. I don't even understand
how edge signalling works. Also, I'd appreciate if you could answer my
comments before respining another version.
As the previous version wasn't based on hierarchy, which I was working on
after your feedback. Apologize!
Thanks,
ÂÂÂÂM.
--
Thanks,
Parthiban N
DENX Software Engineering GmbH, Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-22 Fax: (+49)-8142-66989-80 Email: pn@xxxxxxx