This patch series adds support for the external interrupt controller
(SIRQ) found in the Actions Semi Owl family of SoC's (S500, S700 and
S900). The controller handles up to 3 external interrupt lines through
dedicated SIRQ pins.
This is a rework of the patch series submitted some time ago by
Parthiban Nallathambi:
https://lore.kernel.org/lkml/[email protected]/
Please note I have dropped, for the moment, the S700 related patches
since I do not own a compatible hardware for testing. I'm using instead
an S500 SoC based board for which I have already provided the initial
support:
https://lore.kernel.org/lkml/[email protected]/
The SIRQ controller support is a prerequisite of the soon to be submitted
MFD driver for the Actions Semi ATC260x PMICs.
Thanks and regards,
Cristi
Changes in v5:
- Integrated Marc's review (more details in the driver patch changelog)
- Rebased patch series on v5.9-rc1
Changes in v4:
- Simplified the DTS structure:
* dropped 'actions,sirq-shared-reg' node, now the differentiation
between SoC variants is handled now via the compatible property
* dropped 'actions,sirq-reg-offset', now controller base address in
DTS points to SIRQ0 register, so no additional information is
required for S500 and S700, while for S900 SoC the offsets of SIRQ1
and SIRQ2 regs are provided by the driver
* 'actions,ext-irq-range' was replaced with 'actions,ext-interrupts',
an array of the GIC interrupts triggered by the controller
- Fixed wrong INTC_EXTCTL_TYPE_MASK definition
- Removed redundant irq_fwspec checks in owl_sirq_domain_alloc()
- Improved error handling in owl_sirq_of_init()
- Added yaml binding document
- Dropped S700 related DTS patches for lack of testing hardware:
* arm64: dts: actions: Add sirq node for Actions Semi S700
* arm64: dts: actions: s700-cubieboard7: Enable SIRQ
- Updated MAINTAINERS
- Rebased patchset on kernel v5.8
- Cosmetic changes
* Ordered include statements alphabetically
* Added comment to owl_sirq_set_type() describing conversion of falling
edge or active low signals
* Replaced IRQF_TRIGGER_* with corresponding IRQ_TYPE_* variants
* Ensured data types and function naming are consistent regarding the
'owl_sirq' prefix
Changes in v3 (Parthiban Nallathambi):
- Set default operating frequency to 24MHz
- Falling edge and Low Level interrupts translated to rising edge and high level
- Introduced common function with lock handling for register read and write
- Used direct GIC interrupt number for interrupt local hwirq and finding offset
using DT entry (range) when registers are shared
- Changed irq_ack to irq_eoi
- Added translation method for irq_domain_ops
- Clearing interrupt pending based on bitmask for edge triggered
- Added pinctrl definition for sirq for cubieboard7. This depends on,
https://lore.kernel.org/patchwork/patch/1012859/
Changes in v2 (Parthiban Nallathambi):
- Added SIRQ as hierarchical chip
GIC <----> SIRQ <----> External interrupt controller/Child devices
- Device binding updates with vendor prefix
- Register sharing handled globally and common init sequence/data for all
actions SoC family
Cristian Ciocaltea (3):
dt-bindings: interrupt-controller: Add Actions SIRQ controller binding
irqchip: Add Actions Semi Owl SIRQ controller
MAINTAINERS: Add entries for Actions Semi Owl SIRQ controller
.../actions,owl-sirq.yaml | 68 ++++
MAINTAINERS | 2 +
drivers/irqchip/Makefile | 1 +
drivers/irqchip/irq-owl-sirq.c | 347 ++++++++++++++++++
4 files changed, 418 insertions(+)
create mode 100644 Documentation/devicetree/bindings/interrupt-controller/actions,owl-sirq.yaml
create mode 100644 drivers/irqchip/irq-owl-sirq.c
--
2.28.0
Actions Semi Owl SoCs SIRQ interrupt controller is found in S500, S700
and S900 SoCs and provides support for handling up to 3 external
interrupt lines.
Signed-off-by: Cristian Ciocaltea <[email protected]>
---
Changes in v5:
- Updated controller description statements both in the commit message
and the binding doc
.../actions,owl-sirq.yaml | 68 +++++++++++++++++++
1 file changed, 68 insertions(+)
create mode 100644 Documentation/devicetree/bindings/interrupt-controller/actions,owl-sirq.yaml
diff --git a/Documentation/devicetree/bindings/interrupt-controller/actions,owl-sirq.yaml b/Documentation/devicetree/bindings/interrupt-controller/actions,owl-sirq.yaml
new file mode 100644
index 000000000000..cf9b7a514e4e
--- /dev/null
+++ b/Documentation/devicetree/bindings/interrupt-controller/actions,owl-sirq.yaml
@@ -0,0 +1,68 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/interrupt-controller/actions,owl-sirq.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Actions Semi Owl SoCs SIRQ interrupt controller
+
+maintainers:
+ - Manivannan Sadhasivam <[email protected]>
+ - Cristian Ciocaltea <[email protected]>
+
+description: |
+ This interrupt controller is found in the Actions Semi Owl SoCs (S500, S700
+ and S900) and provides support for handling up to 3 external interrupt lines.
+
+properties:
+ compatible:
+ oneOf:
+ - items:
+ - enum:
+ - actions,s500-sirq
+ - actions,s700-sirq
+ - actions,s900-sirq
+ - const: actions,owl-sirq
+ - const: actions,owl-sirq
+
+ reg:
+ maxItems: 1
+
+ interrupt-controller: true
+
+ '#interrupt-cells':
+ const: 2
+ description:
+ The first cell is the input IRQ number, between 0 and 2, while the second
+ cell is the trigger type as defined in interrupt.txt in this directory.
+
+ 'actions,ext-interrupts':
+ description: |
+ Contains the GIC SPI IRQ numbers mapped to the external interrupt
+ lines. They shall be specified sequentially from output 0 to 2.
+ $ref: /schemas/types.yaml#/definitions/uint32-array
+ minItems: 3
+ maxItems: 3
+
+required:
+ - compatible
+ - reg
+ - interrupt-controller
+ - '#interrupt-cells'
+ - 'actions,ext-interrupts'
+
+additionalProperties: false
+
+examples:
+ - |
+ sirq: interrupt-controller@b01b0200 {
+ compatible = "actions,s500-sirq", "actions,owl-sirq";
+ reg = <0xb01b0200 0x4>;
+ interrupt-controller;
+ #interrupt-cells = <2>;
+ actions,ext-interrupts = <13>, /* SIRQ0 */
+ <14>, /* SIRQ1 */
+ <15>; /* SIRQ2 */
+ };
+
+...
--
2.28.0
Add entries for Actions Semi Owl SIRQ controller binding and driver.
Signed-off-by: Cristian Ciocaltea <[email protected]>
---
MAINTAINERS | 2 ++
1 file changed, 2 insertions(+)
diff --git a/MAINTAINERS b/MAINTAINERS
index db97d048a92e..e28d8ad47d03 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1525,6 +1525,7 @@ F: Documentation/devicetree/bindings/arm/actions.yaml
F: Documentation/devicetree/bindings/clock/actions,owl-cmu.txt
F: Documentation/devicetree/bindings/dma/owl-dma.txt
F: Documentation/devicetree/bindings/i2c/i2c-owl.txt
+F: Documentation/devicetree/bindings/interrupt-controller/actions,owl-sirq.yaml
F: Documentation/devicetree/bindings/mmc/owl-mmc.yaml
F: Documentation/devicetree/bindings/pinctrl/actions,*
F: Documentation/devicetree/bindings/power/actions,owl-sps.txt
@@ -1536,6 +1537,7 @@ F: drivers/clk/actions/
F: drivers/clocksource/timer-owl*
F: drivers/dma/owl-dma.c
F: drivers/i2c/busses/i2c-owl.c
+F: drivers/irqchip/irq-owl-sirq.c
F: drivers/mmc/host/owl-mmc.c
F: drivers/pinctrl/actions/*
F: drivers/soc/actions/
--
2.28.0
This interrupt controller is found in the Actions Semi Owl SoCs (S500,
S700 and S900) and provides support for handling up to 3 external
interrupt lines.
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). Additionally, each line can also be masked individually.
This is based on the patch series submitted by Parthiban Nallathambi:
https://lore.kernel.org/lkml/[email protected]/
Signed-off-by: Parthiban Nallathambi <[email protected]>
Signed-off-by: Saravanan Sekar <[email protected]>
[cristi: optimized DT, various fixes/cleanups/improvements]
Signed-off-by: Cristian Ciocaltea <[email protected]>
---
Changes in v5 - according to Marc's review:
* Updated commit message
* Aligned members in struct owl_sirq_chip_data
* Added naming for SIRQ0 control register offset
* Improved code readability by using FIELD_PREP and FIELD_GET
* Dropped redundant handling of the IRQ trigger information
* Added missing irq_set_affinity to owl_sirq_chip descriptor
drivers/irqchip/Makefile | 1 +
drivers/irqchip/irq-owl-sirq.c | 347 +++++++++++++++++++++++++++++++++
2 files changed, 348 insertions(+)
create mode 100644 drivers/irqchip/irq-owl-sirq.c
diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
index 133f9c45744a..b8eb5b8b766d 100644
--- a/drivers/irqchip/Makefile
+++ b/drivers/irqchip/Makefile
@@ -7,6 +7,7 @@ obj-$(CONFIG_ATH79) += irq-ath79-cpu.o
obj-$(CONFIG_ATH79) += irq-ath79-misc.o
obj-$(CONFIG_ARCH_BCM2835) += irq-bcm2835.o
obj-$(CONFIG_ARCH_BCM2835) += irq-bcm2836.o
+obj-$(CONFIG_ARCH_ACTIONS) += irq-owl-sirq.o
obj-$(CONFIG_DAVINCI_AINTC) += irq-davinci-aintc.o
obj-$(CONFIG_DAVINCI_CP_INTC) += irq-davinci-cp-intc.o
obj-$(CONFIG_EXYNOS_IRQ_COMBINER) += exynos-combiner.o
diff --git a/drivers/irqchip/irq-owl-sirq.c b/drivers/irqchip/irq-owl-sirq.c
new file mode 100644
index 000000000000..2e55984d1a2c
--- /dev/null
+++ b/drivers/irqchip/irq-owl-sirq.c
@@ -0,0 +1,347 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Actions Semi Owl SoCs SIRQ interrupt controller driver
+ *
+ * Copyright (C) 2014 Actions Semi Inc.
+ * David Liu <[email protected]>
+ *
+ * Author: Parthiban Nallathambi <[email protected]>
+ * Author: Saravanan Sekar <[email protected]>
+ * Author: Cristian Ciocaltea <[email protected]>
+ */
+
+#include <linux/bitfield.h>
+#include <linux/interrupt.h>
+#include <linux/irqchip.h>
+#include <linux/of_address.h>
+#include <linux/of_irq.h>
+
+#define NUM_SIRQ 3
+
+#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(7, 6)
+#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))
+
+/* S500 & S700 SIRQ control register masks */
+#define INTC_EXTCTL_SIRQ0_MASK GENMASK(23, 16)
+#define INTC_EXTCTL_SIRQ1_MASK GENMASK(15, 8)
+#define INTC_EXTCTL_SIRQ2_MASK GENMASK(7, 0)
+
+/* S900 SIRQ control register offsets, relative to controller base address */
+#define INTC_EXTCTL0 0x0000
+#define INTC_EXTCTL1 0x0328
+#define INTC_EXTCTL2 0x032c
+
+struct owl_sirq_params {
+ /* INTC_EXTCTL reg shared for all three SIRQ lines */
+ bool reg_shared;
+ /* INTC_EXTCTL reg offsets relative to controller base address */
+ u16 reg_offset[NUM_SIRQ];
+};
+
+struct owl_sirq_chip_data {
+ const struct owl_sirq_params *params;
+ void __iomem *base;
+ raw_spinlock_t lock;
+ u32 ext_irqs[NUM_SIRQ];
+};
+
+/* S500 & S700 SoCs */
+static const struct owl_sirq_params owl_sirq_s500_params = {
+ .reg_shared = true,
+ .reg_offset = { 0, 0, 0 },
+};
+
+/* S900 SoC */
+static const struct owl_sirq_params owl_sirq_s900_params = {
+ .reg_shared = false,
+ .reg_offset = { INTC_EXTCTL0, INTC_EXTCTL1, INTC_EXTCTL2 },
+};
+
+static u32 owl_field_get(u32 val, u32 index)
+{
+ switch (index) {
+ case 0:
+ return FIELD_GET(INTC_EXTCTL_SIRQ0_MASK, val);
+ case 1:
+ return FIELD_GET(INTC_EXTCTL_SIRQ1_MASK, val);
+ case 2:
+ default:
+ return FIELD_GET(INTC_EXTCTL_SIRQ2_MASK, val);
+ }
+}
+
+static u32 owl_field_prep(u32 val, u32 index)
+{
+ switch (index) {
+ case 0:
+ return FIELD_PREP(INTC_EXTCTL_SIRQ0_MASK, val);
+ case 1:
+ return FIELD_PREP(INTC_EXTCTL_SIRQ1_MASK, val);
+ case 2:
+ default:
+ return FIELD_PREP(INTC_EXTCTL_SIRQ2_MASK, val);
+ }
+}
+
+static u32 owl_sirq_read_extctl(struct owl_sirq_chip_data *data, u32 index)
+{
+ u32 val;
+
+ val = readl_relaxed(data->base + data->params->reg_offset[index]);
+ if (data->params->reg_shared)
+ val = owl_field_get(val, index);
+
+ return val;
+}
+
+static void owl_sirq_write_extctl(struct owl_sirq_chip_data *data,
+ u32 extctl, u32 index)
+{
+ u32 val;
+
+ if (data->params->reg_shared) {
+ val = readl_relaxed(data->base + data->params->reg_offset[index]);
+ val &= ~owl_field_prep(0xff, index);
+ extctl = owl_field_prep(extctl, index) | val;
+ }
+
+ writel_relaxed(extctl, data->base + data->params->reg_offset[index]);
+}
+
+static void owl_sirq_clear_set_extctl(struct owl_sirq_chip_data *d,
+ u32 clear, u32 set, u32 index)
+{
+ unsigned long flags;
+ u32 val;
+
+ raw_spin_lock_irqsave(&d->lock, flags);
+ val = owl_sirq_read_extctl(d, index);
+ val &= ~clear;
+ val |= set;
+ owl_sirq_write_extctl(d, val, index);
+ raw_spin_unlock_irqrestore(&d->lock, flags);
+}
+
+static void owl_sirq_eoi(struct irq_data *data)
+{
+ struct owl_sirq_chip_data *chip_data = irq_data_get_irq_chip_data(data);
+
+ /*
+ * Software must clear external interrupt pending, when interrupt type
+ * is edge triggered, so we need per SIRQ based clearing.
+ */
+ if (!irqd_is_level_type(data))
+ owl_sirq_clear_set_extctl(chip_data, 0, INTC_EXTCTL_PENDING,
+ data->hwirq);
+
+ irq_chip_eoi_parent(data);
+}
+
+static void owl_sirq_mask(struct irq_data *data)
+{
+ struct owl_sirq_chip_data *chip_data = irq_data_get_irq_chip_data(data);
+
+ owl_sirq_clear_set_extctl(chip_data, INTC_EXTCTL_EN, 0, data->hwirq);
+ irq_chip_mask_parent(data);
+}
+
+static void owl_sirq_unmask(struct irq_data *data)
+{
+ struct owl_sirq_chip_data *chip_data = irq_data_get_irq_chip_data(data);
+
+ owl_sirq_clear_set_extctl(chip_data, 0, INTC_EXTCTL_EN, data->hwirq);
+ irq_chip_unmask_parent(data);
+}
+
+/*
+ * GIC does not handle falling edge or active low, hence SIRQ shall be
+ * programmed to convert falling edge to rising edge signal and active
+ * low to active high signal.
+ */
+static int owl_sirq_set_type(struct irq_data *data, unsigned int type)
+{
+ struct owl_sirq_chip_data *chip_data = irq_data_get_irq_chip_data(data);
+ u32 sirq_type;
+
+ switch (type) {
+ case IRQ_TYPE_LEVEL_LOW:
+ sirq_type = INTC_EXTCTL_TYPE_LOW;
+ type = IRQ_TYPE_LEVEL_HIGH;
+ break;
+ case IRQ_TYPE_LEVEL_HIGH:
+ sirq_type = INTC_EXTCTL_TYPE_HIGH;
+ break;
+ case IRQ_TYPE_EDGE_FALLING:
+ sirq_type = INTC_EXTCTL_TYPE_FALLING;
+ type = IRQ_TYPE_EDGE_RISING;
+ break;
+ case IRQ_TYPE_EDGE_RISING:
+ sirq_type = INTC_EXTCTL_TYPE_RISING;
+ break;
+ default:
+ return -EINVAL;
+ }
+
+ owl_sirq_clear_set_extctl(chip_data, INTC_EXTCTL_TYPE_MASK, sirq_type,
+ data->hwirq);
+
+ return irq_chip_set_type_parent(data, type);
+}
+
+static struct irq_chip owl_sirq_chip = {
+ .name = "owl-sirq",
+ .irq_mask = owl_sirq_mask,
+ .irq_unmask = owl_sirq_unmask,
+ .irq_eoi = owl_sirq_eoi,
+ .irq_set_type = owl_sirq_set_type,
+ .irq_retrigger = irq_chip_retrigger_hierarchy,
+#ifdef CONFIG_SMP
+ .irq_set_affinity = irq_chip_set_affinity_parent,
+#endif
+};
+
+static int owl_sirq_domain_translate(struct irq_domain *d,
+ struct irq_fwspec *fwspec,
+ unsigned long *hwirq,
+ unsigned int *type)
+{
+ if (!is_of_node(fwspec->fwnode))
+ return -EINVAL;
+
+ if (fwspec->param_count != 2 || fwspec->param[0] >= NUM_SIRQ)
+ return -EINVAL;
+
+ *hwirq = fwspec->param[0];
+ *type = fwspec->param[1];
+
+ return 0;
+}
+
+static int owl_sirq_domain_alloc(struct irq_domain *domain, unsigned int virq,
+ unsigned int nr_irqs, void *data)
+{
+ struct owl_sirq_chip_data *chip_data = domain->host_data;
+ struct irq_fwspec *fwspec = data;
+ struct irq_fwspec parent_fwspec;
+ irq_hw_number_t hwirq;
+ unsigned int type;
+ int ret;
+
+ if (WARN_ON(nr_irqs != 1))
+ return -EINVAL;
+
+ ret = owl_sirq_domain_translate(domain, fwspec, &hwirq, &type);
+ if (ret)
+ return ret;
+
+ switch (type) {
+ case IRQ_TYPE_EDGE_RISING:
+ case IRQ_TYPE_LEVEL_HIGH:
+ break;
+ case IRQ_TYPE_EDGE_FALLING:
+ type = IRQ_TYPE_EDGE_RISING;
+ break;
+ case IRQ_TYPE_LEVEL_LOW:
+ type = IRQ_TYPE_LEVEL_HIGH;
+ break;
+ default:
+ return -EINVAL;
+ }
+
+ irq_domain_set_hwirq_and_chip(domain, virq, hwirq, &owl_sirq_chip,
+ chip_data);
+
+ parent_fwspec.fwnode = domain->parent->fwnode;
+ parent_fwspec.param_count = 3;
+ parent_fwspec.param[0] = 0; /* SPI */
+ parent_fwspec.param[1] = chip_data->ext_irqs[hwirq];
+ parent_fwspec.param[2] = type;
+
+ return irq_domain_alloc_irqs_parent(domain, virq, 1, &parent_fwspec);
+}
+
+static const struct irq_domain_ops owl_sirq_domain_ops = {
+ .translate = owl_sirq_domain_translate,
+ .alloc = owl_sirq_domain_alloc,
+ .free = irq_domain_free_irqs_common,
+};
+
+static const struct of_device_id owl_sirq_of_match[] = {
+ { .compatible = "actions,s500-sirq", .data = &owl_sirq_s500_params },
+ { .compatible = "actions,s700-sirq", .data = &owl_sirq_s500_params },
+ { .compatible = "actions,s900-sirq", .data = &owl_sirq_s900_params },
+ { /* sentinel */ }
+};
+
+static int __init owl_sirq_of_init(struct device_node *node,
+ struct device_node *parent)
+{
+ const struct of_device_id *match;
+ struct irq_domain *domain, *parent_domain;
+ struct owl_sirq_chip_data *chip_data;
+ int ret, i;
+
+ parent_domain = irq_find_host(parent);
+ if (!parent_domain) {
+ pr_err("%pOF: failed to find sirq parent domain\n", node);
+ return -ENXIO;
+ }
+
+ chip_data = kzalloc(sizeof(*chip_data), GFP_KERNEL);
+ if (!chip_data)
+ return -ENOMEM;
+
+ match = of_match_node(owl_sirq_of_match, node);
+ if (!match) {
+ pr_warn("owl-sirq: assuming S500/S700 compatible controller\n");
+ chip_data->params = &owl_sirq_s500_params;
+ } else {
+ chip_data->params = match->data;
+ }
+
+ raw_spin_lock_init(&chip_data->lock);
+
+ chip_data->base = of_iomap(node, 0);
+ if (!chip_data->base) {
+ pr_err("%pOF: failed to map sirq registers\n", node);
+ ret = -ENXIO;
+ goto out_free;
+ }
+
+ ret = of_property_read_variable_u32_array(node, "actions,ext-interrupts",
+ chip_data->ext_irqs,
+ NUM_SIRQ, NUM_SIRQ);
+ if (ret < NUM_SIRQ) {
+ pr_err("%pOF: failed to read sirq interrupts\n", node);
+ goto out_unmap;
+ }
+
+ /* Set 24MHz external interrupt clock freq */
+ for (i = 0; i < NUM_SIRQ; i++)
+ owl_sirq_clear_set_extctl(chip_data, 0, INTC_EXTCTL_CLK_SEL, i);
+
+ domain = irq_domain_add_hierarchy(parent_domain, 0, NUM_SIRQ, node,
+ &owl_sirq_domain_ops, chip_data);
+ if (!domain) {
+ pr_err("%pOF: failed to add domain\n", node);
+ ret = -ENOMEM;
+ goto out_unmap;
+ }
+
+ return 0;
+
+out_unmap:
+ iounmap(chip_data->base);
+out_free:
+ kfree(chip_data);
+
+ return ret;
+}
+
+IRQCHIP_DECLARE(owl_sirq, "actions,owl-sirq", owl_sirq_of_init);
--
2.28.0
Hi Cristi,
On Wed, Aug 19, 2020 at 07:37:55PM +0300, Cristian Ciocaltea wrote:
> This patch series adds support for the external interrupt controller
> (SIRQ) found in the Actions Semi Owl family of SoC's (S500, S700 and
> S900). The controller handles up to 3 external interrupt lines through
> dedicated SIRQ pins.
>
> This is a rework of the patch series submitted some time ago by
> Parthiban Nallathambi:
> https://lore.kernel.org/lkml/[email protected]/
>
You need to preserve the authorship while reposting the patches. If you'd
like to take the authorship intentionally then please explain the reason in
cover letter.
Thanks,
Mani
> Please note I have dropped, for the moment, the S700 related patches
> since I do not own a compatible hardware for testing. I'm using instead
> an S500 SoC based board for which I have already provided the initial
> support:
> https://lore.kernel.org/lkml/[email protected]/
>
> The SIRQ controller support is a prerequisite of the soon to be submitted
> MFD driver for the Actions Semi ATC260x PMICs.
>
> Thanks and regards,
> Cristi
>
> Changes in v5:
> - Integrated Marc's review (more details in the driver patch changelog)
> - Rebased patch series on v5.9-rc1
>
> Changes in v4:
> - Simplified the DTS structure:
> * dropped 'actions,sirq-shared-reg' node, now the differentiation
> between SoC variants is handled now via the compatible property
> * dropped 'actions,sirq-reg-offset', now controller base address in
> DTS points to SIRQ0 register, so no additional information is
> required for S500 and S700, while for S900 SoC the offsets of SIRQ1
> and SIRQ2 regs are provided by the driver
> * 'actions,ext-irq-range' was replaced with 'actions,ext-interrupts',
> an array of the GIC interrupts triggered by the controller
> - Fixed wrong INTC_EXTCTL_TYPE_MASK definition
> - Removed redundant irq_fwspec checks in owl_sirq_domain_alloc()
> - Improved error handling in owl_sirq_of_init()
> - Added yaml binding document
> - Dropped S700 related DTS patches for lack of testing hardware:
> * arm64: dts: actions: Add sirq node for Actions Semi S700
> * arm64: dts: actions: s700-cubieboard7: Enable SIRQ
> - Updated MAINTAINERS
> - Rebased patchset on kernel v5.8
> - Cosmetic changes
> * Ordered include statements alphabetically
> * Added comment to owl_sirq_set_type() describing conversion of falling
> edge or active low signals
> * Replaced IRQF_TRIGGER_* with corresponding IRQ_TYPE_* variants
> * Ensured data types and function naming are consistent regarding the
> 'owl_sirq' prefix
>
> Changes in v3 (Parthiban Nallathambi):
> - Set default operating frequency to 24MHz
> - Falling edge and Low Level interrupts translated to rising edge and high level
> - Introduced common function with lock handling for register read and write
> - Used direct GIC interrupt number for interrupt local hwirq and finding offset
> using DT entry (range) when registers are shared
> - Changed irq_ack to irq_eoi
> - Added translation method for irq_domain_ops
> - Clearing interrupt pending based on bitmask for edge triggered
> - Added pinctrl definition for sirq for cubieboard7. This depends on,
> https://lore.kernel.org/patchwork/patch/1012859/
>
> Changes in v2 (Parthiban Nallathambi):
> - Added SIRQ as hierarchical chip
> GIC <----> SIRQ <----> External interrupt controller/Child devices
> - Device binding updates with vendor prefix
> - Register sharing handled globally and common init sequence/data for all
> actions SoC family
>
> Cristian Ciocaltea (3):
> dt-bindings: interrupt-controller: Add Actions SIRQ controller binding
> irqchip: Add Actions Semi Owl SIRQ controller
> MAINTAINERS: Add entries for Actions Semi Owl SIRQ controller
>
> .../actions,owl-sirq.yaml | 68 ++++
> MAINTAINERS | 2 +
> drivers/irqchip/Makefile | 1 +
> drivers/irqchip/irq-owl-sirq.c | 347 ++++++++++++++++++
> 4 files changed, 418 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/interrupt-controller/actions,owl-sirq.yaml
> create mode 100644 drivers/irqchip/irq-owl-sirq.c
>
> --
> 2.28.0
>
Hi Mani,
On Sat, Aug 22, 2020 at 06:47:12PM +0530, Manivannan Sadhasivam wrote:
> Hi Cristi,
>
> On Wed, Aug 19, 2020 at 07:37:55PM +0300, Cristian Ciocaltea wrote:
> > This patch series adds support for the external interrupt controller
> > (SIRQ) found in the Actions Semi Owl family of SoC's (S500, S700 and
> > S900). The controller handles up to 3 external interrupt lines through
> > dedicated SIRQ pins.
> >
> > This is a rework of the patch series submitted some time ago by
> > Parthiban Nallathambi:
> > https://lore.kernel.org/lkml/[email protected]/
> >
>
> You need to preserve the authorship while reposting the patches. If you'd
> like to take the authorship intentionally then please explain the reason in
> cover letter.
>
> Thanks,
> Mani
Thanks for pointing this out, I was not aware of the procedure - this is
actually my very first repost. Could you please indicate how should I
proceed to fix this? I had absolutely no intention to take the authorship..
Sorry for the mistake,
Cristi
> > Please note I have dropped, for the moment, the S700 related patches
> > since I do not own a compatible hardware for testing. I'm using instead
> > an S500 SoC based board for which I have already provided the initial
> > support:
> > https://lore.kernel.org/lkml/[email protected]/
> >
> > The SIRQ controller support is a prerequisite of the soon to be submitted
> > MFD driver for the Actions Semi ATC260x PMICs.
> >
> > Thanks and regards,
> > Cristi
> >
> > Changes in v5:
> > - Integrated Marc's review (more details in the driver patch changelog)
> > - Rebased patch series on v5.9-rc1
> >
> > Changes in v4:
> > - Simplified the DTS structure:
> > * dropped 'actions,sirq-shared-reg' node, now the differentiation
> > between SoC variants is handled now via the compatible property
> > * dropped 'actions,sirq-reg-offset', now controller base address in
> > DTS points to SIRQ0 register, so no additional information is
> > required for S500 and S700, while for S900 SoC the offsets of SIRQ1
> > and SIRQ2 regs are provided by the driver
> > * 'actions,ext-irq-range' was replaced with 'actions,ext-interrupts',
> > an array of the GIC interrupts triggered by the controller
> > - Fixed wrong INTC_EXTCTL_TYPE_MASK definition
> > - Removed redundant irq_fwspec checks in owl_sirq_domain_alloc()
> > - Improved error handling in owl_sirq_of_init()
> > - Added yaml binding document
> > - Dropped S700 related DTS patches for lack of testing hardware:
> > * arm64: dts: actions: Add sirq node for Actions Semi S700
> > * arm64: dts: actions: s700-cubieboard7: Enable SIRQ
> > - Updated MAINTAINERS
> > - Rebased patchset on kernel v5.8
> > - Cosmetic changes
> > * Ordered include statements alphabetically
> > * Added comment to owl_sirq_set_type() describing conversion of falling
> > edge or active low signals
> > * Replaced IRQF_TRIGGER_* with corresponding IRQ_TYPE_* variants
> > * Ensured data types and function naming are consistent regarding the
> > 'owl_sirq' prefix
> >
> > Changes in v3 (Parthiban Nallathambi):
> > - Set default operating frequency to 24MHz
> > - Falling edge and Low Level interrupts translated to rising edge and high level
> > - Introduced common function with lock handling for register read and write
> > - Used direct GIC interrupt number for interrupt local hwirq and finding offset
> > using DT entry (range) when registers are shared
> > - Changed irq_ack to irq_eoi
> > - Added translation method for irq_domain_ops
> > - Clearing interrupt pending based on bitmask for edge triggered
> > - Added pinctrl definition for sirq for cubieboard7. This depends on,
> > https://lore.kernel.org/patchwork/patch/1012859/
> >
> > Changes in v2 (Parthiban Nallathambi):
> > - Added SIRQ as hierarchical chip
> > GIC <----> SIRQ <----> External interrupt controller/Child devices
> > - Device binding updates with vendor prefix
> > - Register sharing handled globally and common init sequence/data for all
> > actions SoC family
> >
> > Cristian Ciocaltea (3):
> > dt-bindings: interrupt-controller: Add Actions SIRQ controller binding
> > irqchip: Add Actions Semi Owl SIRQ controller
> > MAINTAINERS: Add entries for Actions Semi Owl SIRQ controller
> >
> > .../actions,owl-sirq.yaml | 68 ++++
> > MAINTAINERS | 2 +
> > drivers/irqchip/Makefile | 1 +
> > drivers/irqchip/irq-owl-sirq.c | 347 ++++++++++++++++++
> > 4 files changed, 418 insertions(+)
> > create mode 100644 Documentation/devicetree/bindings/interrupt-controller/actions,owl-sirq.yaml
> > create mode 100644 drivers/irqchip/irq-owl-sirq.c
> >
> > --
> > 2.28.0
> >
On 23 August 2020 4:35:13 AM IST, Cristian Ciocaltea <[email protected]> wrote:
>Hi Mani,
>
>On Sat, Aug 22, 2020 at 06:47:12PM +0530, Manivannan Sadhasivam wrote:
>> Hi Cristi,
>>
>> On Wed, Aug 19, 2020 at 07:37:55PM +0300, Cristian Ciocaltea wrote:
>> > This patch series adds support for the external interrupt
>controller
>> > (SIRQ) found in the Actions Semi Owl family of SoC's (S500, S700
>and
>> > S900). The controller handles up to 3 external interrupt lines
>through
>> > dedicated SIRQ pins.
>> >
>> > This is a rework of the patch series submitted some time ago by
>> > Parthiban Nallathambi:
>> > https://lore.kernel.org/lkml/[email protected]/
>> >
>>
>> You need to preserve the authorship while reposting the patches. If
>you'd
>> like to take the authorship intentionally then please explain the
>reason in
>> cover letter.
>>
>> Thanks,
>> Mani
>
>Thanks for pointing this out, I was not aware of the procedure - this
>is
>actually my very first repost. Could you please indicate how should I
>proceed to fix this? I had absolutely no intention to take the
>authorship..
>
Below command would change the author of last commit:
git commit --amend --author="Manivannan Sadhasivam <[email protected]>"
>Sorry for the mistake,
No issues!
Thanks,
Mani
>Cristi
>
>> > Please note I have dropped, for the moment, the S700 related
>patches
>> > since I do not own a compatible hardware for testing. I'm using
>instead
>> > an S500 SoC based board for which I have already provided the
>initial
>> > support:
>> >
>https://lore.kernel.org/lkml/[email protected]/
>> >
>> > The SIRQ controller support is a prerequisite of the soon to be
>submitted
>> > MFD driver for the Actions Semi ATC260x PMICs.
>> >
>> > Thanks and regards,
>> > Cristi
>> >
>> > Changes in v5:
>> > - Integrated Marc's review (more details in the driver patch
>changelog)
>> > - Rebased patch series on v5.9-rc1
>> >
>> > Changes in v4:
>> > - Simplified the DTS structure:
>> > * dropped 'actions,sirq-shared-reg' node, now the differentiation
>> > between SoC variants is handled now via the compatible property
>> > * dropped 'actions,sirq-reg-offset', now controller base address
>in
>> > DTS points to SIRQ0 register, so no additional information is
>> > required for S500 and S700, while for S900 SoC the offsets of
>SIRQ1
>> > and SIRQ2 regs are provided by the driver
>> > * 'actions,ext-irq-range' was replaced with
>'actions,ext-interrupts',
>> > an array of the GIC interrupts triggered by the controller
>> > - Fixed wrong INTC_EXTCTL_TYPE_MASK definition
>> > - Removed redundant irq_fwspec checks in owl_sirq_domain_alloc()
>> > - Improved error handling in owl_sirq_of_init()
>> > - Added yaml binding document
>> > - Dropped S700 related DTS patches for lack of testing hardware:
>> > * arm64: dts: actions: Add sirq node for Actions Semi S700
>> > * arm64: dts: actions: s700-cubieboard7: Enable SIRQ
>> > - Updated MAINTAINERS
>> > - Rebased patchset on kernel v5.8
>> > - Cosmetic changes
>> > * Ordered include statements alphabetically
>> > * Added comment to owl_sirq_set_type() describing conversion of
>falling
>> > edge or active low signals
>> > * Replaced IRQF_TRIGGER_* with corresponding IRQ_TYPE_* variants
>> > * Ensured data types and function naming are consistent regarding
>the
>> > 'owl_sirq' prefix
>> >
>> > Changes in v3 (Parthiban Nallathambi):
>> > - Set default operating frequency to 24MHz
>> > - Falling edge and Low Level interrupts translated to rising edge
>and high level
>> > - Introduced common function with lock handling for register read
>and write
>> > - Used direct GIC interrupt number for interrupt local hwirq and
>finding offset
>> > using DT entry (range) when registers are shared
>> > - Changed irq_ack to irq_eoi
>> > - Added translation method for irq_domain_ops
>> > - Clearing interrupt pending based on bitmask for edge triggered
>> > - Added pinctrl definition for sirq for cubieboard7. This depends
>on,
>> > https://lore.kernel.org/patchwork/patch/1012859/
>> >
>> > Changes in v2 (Parthiban Nallathambi):
>> > - Added SIRQ as hierarchical chip
>> > GIC <----> SIRQ <----> External interrupt controller/Child
>devices
>> > - Device binding updates with vendor prefix
>> > - Register sharing handled globally and common init sequence/data
>for all
>> > actions SoC family
>> >
>> > Cristian Ciocaltea (3):
>> > dt-bindings: interrupt-controller: Add Actions SIRQ controller
>binding
>> > irqchip: Add Actions Semi Owl SIRQ controller
>> > MAINTAINERS: Add entries for Actions Semi Owl SIRQ controller
>> >
>> > .../actions,owl-sirq.yaml | 68 ++++
>> > MAINTAINERS | 2 +
>> > drivers/irqchip/Makefile | 1 +
>> > drivers/irqchip/irq-owl-sirq.c | 347
>++++++++++++++++++
>> > 4 files changed, 418 insertions(+)
>> > create mode 100644
>Documentation/devicetree/bindings/interrupt-controller/actions,owl-sirq.yaml
>> > create mode 100644 drivers/irqchip/irq-owl-sirq.c
>> >
>> > --
>> > 2.28.0
>> >
--
Sent from my Android device with K-9 Mail. Please excuse my brevity.
On Tue, Aug 25, 2020 at 07:39:32AM +0530, Manivannan Sadhasivam wrote:
>
>
> On 23 August 2020 4:35:13 AM IST, Cristian Ciocaltea <[email protected]> wrote:
> >Hi Mani,
> >
> >On Sat, Aug 22, 2020 at 06:47:12PM +0530, Manivannan Sadhasivam wrote:
> >> Hi Cristi,
> >>
> >> On Wed, Aug 19, 2020 at 07:37:55PM +0300, Cristian Ciocaltea wrote:
> >> > This patch series adds support for the external interrupt
> >controller
> >> > (SIRQ) found in the Actions Semi Owl family of SoC's (S500, S700
> >and
> >> > S900). The controller handles up to 3 external interrupt lines
> >through
> >> > dedicated SIRQ pins.
> >> >
> >> > This is a rework of the patch series submitted some time ago by
> >> > Parthiban Nallathambi:
> >> > https://lore.kernel.org/lkml/[email protected]/
> >> >
> >>
> >> You need to preserve the authorship while reposting the patches. If
> >you'd
> >> like to take the authorship intentionally then please explain the
> >reason in
> >> cover letter.
> >>
> >> Thanks,
> >> Mani
> >
> >Thanks for pointing this out, I was not aware of the procedure - this
> >is
> >actually my very first repost. Could you please indicate how should I
> >proceed to fix this? I had absolutely no intention to take the
> >authorship..
> >
>
> Below command would change the author of last commit:
>
> git commit --amend --author="Manivannan Sadhasivam <[email protected]>"
Got it now, for some reason I have lost original author information in
my local repository - I'll pay more attention to this next time.
I will submit the fix after receiving Rob's review for the binding doc,
if that would be ok.
Thanks,
Cristi
> >Sorry for the mistake,
>
> No issues!
>
> Thanks,
> Mani
>
> >Cristi
> >
> >> > Please note I have dropped, for the moment, the S700 related
> >patches
> >> > since I do not own a compatible hardware for testing. I'm using
> >instead
> >> > an S500 SoC based board for which I have already provided the
> >initial
> >> > support:
> >> >
> >https://lore.kernel.org/lkml/[email protected]/
> >> >
> >> > The SIRQ controller support is a prerequisite of the soon to be
> >submitted
> >> > MFD driver for the Actions Semi ATC260x PMICs.
> >> >
> >> > Thanks and regards,
> >> > Cristi
> >> >
> >> > Changes in v5:
> >> > - Integrated Marc's review (more details in the driver patch
> >changelog)
> >> > - Rebased patch series on v5.9-rc1
> >> >
> >> > Changes in v4:
> >> > - Simplified the DTS structure:
> >> > * dropped 'actions,sirq-shared-reg' node, now the differentiation
> >> > between SoC variants is handled now via the compatible property
> >> > * dropped 'actions,sirq-reg-offset', now controller base address
> >in
> >> > DTS points to SIRQ0 register, so no additional information is
> >> > required for S500 and S700, while for S900 SoC the offsets of
> >SIRQ1
> >> > and SIRQ2 regs are provided by the driver
> >> > * 'actions,ext-irq-range' was replaced with
> >'actions,ext-interrupts',
> >> > an array of the GIC interrupts triggered by the controller
> >> > - Fixed wrong INTC_EXTCTL_TYPE_MASK definition
> >> > - Removed redundant irq_fwspec checks in owl_sirq_domain_alloc()
> >> > - Improved error handling in owl_sirq_of_init()
> >> > - Added yaml binding document
> >> > - Dropped S700 related DTS patches for lack of testing hardware:
> >> > * arm64: dts: actions: Add sirq node for Actions Semi S700
> >> > * arm64: dts: actions: s700-cubieboard7: Enable SIRQ
> >> > - Updated MAINTAINERS
> >> > - Rebased patchset on kernel v5.8
> >> > - Cosmetic changes
> >> > * Ordered include statements alphabetically
> >> > * Added comment to owl_sirq_set_type() describing conversion of
> >falling
> >> > edge or active low signals
> >> > * Replaced IRQF_TRIGGER_* with corresponding IRQ_TYPE_* variants
> >> > * Ensured data types and function naming are consistent regarding
> >the
> >> > 'owl_sirq' prefix
> >> >
> >> > Changes in v3 (Parthiban Nallathambi):
> >> > - Set default operating frequency to 24MHz
> >> > - Falling edge and Low Level interrupts translated to rising edge
> >and high level
> >> > - Introduced common function with lock handling for register read
> >and write
> >> > - Used direct GIC interrupt number for interrupt local hwirq and
> >finding offset
> >> > using DT entry (range) when registers are shared
> >> > - Changed irq_ack to irq_eoi
> >> > - Added translation method for irq_domain_ops
> >> > - Clearing interrupt pending based on bitmask for edge triggered
> >> > - Added pinctrl definition for sirq for cubieboard7. This depends
> >on,
> >> > https://lore.kernel.org/patchwork/patch/1012859/
> >> >
> >> > Changes in v2 (Parthiban Nallathambi):
> >> > - Added SIRQ as hierarchical chip
> >> > GIC <----> SIRQ <----> External interrupt controller/Child
> >devices
> >> > - Device binding updates with vendor prefix
> >> > - Register sharing handled globally and common init sequence/data
> >for all
> >> > actions SoC family
> >> >
> >> > Cristian Ciocaltea (3):
> >> > dt-bindings: interrupt-controller: Add Actions SIRQ controller
> >binding
> >> > irqchip: Add Actions Semi Owl SIRQ controller
> >> > MAINTAINERS: Add entries for Actions Semi Owl SIRQ controller
> >> >
> >> > .../actions,owl-sirq.yaml | 68 ++++
> >> > MAINTAINERS | 2 +
> >> > drivers/irqchip/Makefile | 1 +
> >> > drivers/irqchip/irq-owl-sirq.c | 347
> >++++++++++++++++++
> >> > 4 files changed, 418 insertions(+)
> >> > create mode 100644
> >Documentation/devicetree/bindings/interrupt-controller/actions,owl-sirq.yaml
> >> > create mode 100644 drivers/irqchip/irq-owl-sirq.c
> >> >
> >> > --
> >> > 2.28.0
> >> >
>
> --
> Sent from my Android device with K-9 Mail. Please excuse my brevity.
On Wed, Aug 19, 2020 at 07:37:56PM +0300, Cristian Ciocaltea wrote:
> Actions Semi Owl SoCs SIRQ interrupt controller is found in S500, S700
> and S900 SoCs and provides support for handling up to 3 external
> interrupt lines.
>
> Signed-off-by: Cristian Ciocaltea <[email protected]>
> ---
> Changes in v5:
> - Updated controller description statements both in the commit message
> and the binding doc
>
> .../actions,owl-sirq.yaml | 68 +++++++++++++++++++
> 1 file changed, 68 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/interrupt-controller/actions,owl-sirq.yaml
>
> diff --git a/Documentation/devicetree/bindings/interrupt-controller/actions,owl-sirq.yaml b/Documentation/devicetree/bindings/interrupt-controller/actions,owl-sirq.yaml
> new file mode 100644
> index 000000000000..cf9b7a514e4e
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/interrupt-controller/actions,owl-sirq.yaml
> @@ -0,0 +1,68 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/interrupt-controller/actions,owl-sirq.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Actions Semi Owl SoCs SIRQ interrupt controller
> +
> +maintainers:
> + - Manivannan Sadhasivam <[email protected]>
> + - Cristian Ciocaltea <[email protected]>
> +
> +description: |
> + This interrupt controller is found in the Actions Semi Owl SoCs (S500, S700
> + and S900) and provides support for handling up to 3 external interrupt lines.
> +
> +properties:
> + compatible:
> + oneOf:
> + - items:
> + - enum:
> + - actions,s500-sirq
> + - actions,s700-sirq
> + - actions,s900-sirq
> + - const: actions,owl-sirq
> + - const: actions,owl-sirq
This should be dropped. You should always have the SoC specific
compatible.
> +
> + reg:
> + maxItems: 1
> +
> + interrupt-controller: true
> +
> + '#interrupt-cells':
> + const: 2
> + description:
> + The first cell is the input IRQ number, between 0 and 2, while the second
> + cell is the trigger type as defined in interrupt.txt in this directory.
> +
> + 'actions,ext-interrupts':
> + description: |
> + Contains the GIC SPI IRQ numbers mapped to the external interrupt
> + lines. They shall be specified sequentially from output 0 to 2.
> + $ref: /schemas/types.yaml#/definitions/uint32-array
> + minItems: 3
> + maxItems: 3
Can't you use 'interrupts' here?
> +
> +required:
> + - compatible
> + - reg
> + - interrupt-controller
> + - '#interrupt-cells'
> + - 'actions,ext-interrupts'
> +
> +additionalProperties: false
> +
> +examples:
> + - |
> + sirq: interrupt-controller@b01b0200 {
> + compatible = "actions,s500-sirq", "actions,owl-sirq";
> + reg = <0xb01b0200 0x4>;
> + interrupt-controller;
> + #interrupt-cells = <2>;
> + actions,ext-interrupts = <13>, /* SIRQ0 */
> + <14>, /* SIRQ1 */
> + <15>; /* SIRQ2 */
> + };
> +
> +...
> --
> 2.28.0
>
Hi Rob,
Thanks for the review!
On Tue, Aug 25, 2020 at 04:09:13PM -0600, Rob Herring wrote:
> On Wed, Aug 19, 2020 at 07:37:56PM +0300, Cristian Ciocaltea wrote:
> > Actions Semi Owl SoCs SIRQ interrupt controller is found in S500, S700
> > and S900 SoCs and provides support for handling up to 3 external
> > interrupt lines.
> >
> > Signed-off-by: Cristian Ciocaltea <[email protected]>
> > ---
> > Changes in v5:
> > - Updated controller description statements both in the commit message
> > and the binding doc
> >
> > .../actions,owl-sirq.yaml | 68 +++++++++++++++++++
> > 1 file changed, 68 insertions(+)
> > create mode 100644 Documentation/devicetree/bindings/interrupt-controller/actions,owl-sirq.yaml
> >
> > diff --git a/Documentation/devicetree/bindings/interrupt-controller/actions,owl-sirq.yaml b/Documentation/devicetree/bindings/interrupt-controller/actions,owl-sirq.yaml
> > new file mode 100644
> > index 000000000000..cf9b7a514e4e
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/interrupt-controller/actions,owl-sirq.yaml
> > @@ -0,0 +1,68 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/interrupt-controller/actions,owl-sirq.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Actions Semi Owl SoCs SIRQ interrupt controller
> > +
> > +maintainers:
> > + - Manivannan Sadhasivam <[email protected]>
> > + - Cristian Ciocaltea <[email protected]>
> > +
> > +description: |
> > + This interrupt controller is found in the Actions Semi Owl SoCs (S500, S700
> > + and S900) and provides support for handling up to 3 external interrupt lines.
> > +
> > +properties:
> > + compatible:
> > + oneOf:
> > + - items:
> > + - enum:
> > + - actions,s500-sirq
> > + - actions,s700-sirq
> > + - actions,s900-sirq
> > + - const: actions,owl-sirq
> > + - const: actions,owl-sirq
>
> This should be dropped. You should always have the SoC specific
> compatible.
Sure, I will get rid of the 'owl-sirq' compatible.
> > +
> > + reg:
> > + maxItems: 1
> > +
> > + interrupt-controller: true
> > +
> > + '#interrupt-cells':
> > + const: 2
> > + description:
> > + The first cell is the input IRQ number, between 0 and 2, while the second
> > + cell is the trigger type as defined in interrupt.txt in this directory.
> > +
> > + 'actions,ext-interrupts':
> > + description: |
> > + Contains the GIC SPI IRQ numbers mapped to the external interrupt
> > + lines. They shall be specified sequentially from output 0 to 2.
> > + $ref: /schemas/types.yaml#/definitions/uint32-array
> > + minItems: 3
> > + maxItems: 3
>
> Can't you use 'interrupts' here?
This was actually my initial idea, but it might confuse the users since
this is not following the parent controller IRQ specs, i.e. the trigger
type is set internally by the SIRQ driver, it's not taken from DT.
Please see the DTS sample bellow where both devices are on the same
level and have GIC as interrupt parent. The 'interrupts' property
in the sirq node looks incomplete now. That is why I decided to use
a custom name for it, although I'm not sure it's the most relevant one,
I am open to any other suggestion.
i2c0: i2c@b0170000 {
[...]
interrupts = <GIC_SPI 25 IRQ_TYPE_LEVEL_HIGH>;
[...]
};
sirq: interrupt-controller@b01b0200 {
[...]
interrupt-controller;
#interrupt-cells = <2>;
interrupts = <13>, /* SIRQ0 */
<14>, /* SIRQ1 */
<15>; /* SIRQ2 */
};
Regards,
Cristi
> > +
> > +required:
> > + - compatible
> > + - reg
> > + - interrupt-controller
> > + - '#interrupt-cells'
> > + - 'actions,ext-interrupts'
> > +
> > +additionalProperties: false
> > +
> > +examples:
> > + - |
> > + sirq: interrupt-controller@b01b0200 {
> > + compatible = "actions,s500-sirq", "actions,owl-sirq";
> > + reg = <0xb01b0200 0x4>;
> > + interrupt-controller;
> > + #interrupt-cells = <2>;
> > + actions,ext-interrupts = <13>, /* SIRQ0 */
> > + <14>, /* SIRQ1 */
> > + <15>; /* SIRQ2 */
> > + };
> > +
> > +...
> > --
> > 2.28.0
> >
On Wed, Aug 26, 2020 at 3:42 PM Cristian Ciocaltea
<[email protected]> wrote:
>
> Hi Rob,
>
> Thanks for the review!
>
> On Tue, Aug 25, 2020 at 04:09:13PM -0600, Rob Herring wrote:
> > On Wed, Aug 19, 2020 at 07:37:56PM +0300, Cristian Ciocaltea wrote:
> > > Actions Semi Owl SoCs SIRQ interrupt controller is found in S500, S700
> > > and S900 SoCs and provides support for handling up to 3 external
> > > interrupt lines.
> > >
> > > Signed-off-by: Cristian Ciocaltea <[email protected]>
> > > ---
> > > Changes in v5:
> > > - Updated controller description statements both in the commit message
> > > and the binding doc
> > >
> > > .../actions,owl-sirq.yaml | 68 +++++++++++++++++++
> > > 1 file changed, 68 insertions(+)
> > > create mode 100644 Documentation/devicetree/bindings/interrupt-controller/actions,owl-sirq.yaml
> > >
> > > diff --git a/Documentation/devicetree/bindings/interrupt-controller/actions,owl-sirq.yaml b/Documentation/devicetree/bindings/interrupt-controller/actions,owl-sirq.yaml
> > > new file mode 100644
> > > index 000000000000..cf9b7a514e4e
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/interrupt-controller/actions,owl-sirq.yaml
> > > @@ -0,0 +1,68 @@
> > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > > +%YAML 1.2
> > > +---
> > > +$id: http://devicetree.org/schemas/interrupt-controller/actions,owl-sirq.yaml#
> > > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > +
> > > +title: Actions Semi Owl SoCs SIRQ interrupt controller
> > > +
> > > +maintainers:
> > > + - Manivannan Sadhasivam <[email protected]>
> > > + - Cristian Ciocaltea <[email protected]>
> > > +
> > > +description: |
> > > + This interrupt controller is found in the Actions Semi Owl SoCs (S500, S700
> > > + and S900) and provides support for handling up to 3 external interrupt lines.
> > > +
> > > +properties:
> > > + compatible:
> > > + oneOf:
> > > + - items:
> > > + - enum:
> > > + - actions,s500-sirq
> > > + - actions,s700-sirq
> > > + - actions,s900-sirq
> > > + - const: actions,owl-sirq
> > > + - const: actions,owl-sirq
> >
> > This should be dropped. You should always have the SoC specific
> > compatible.
>
> Sure, I will get rid of the 'owl-sirq' compatible.
>
> > > +
> > > + reg:
> > > + maxItems: 1
> > > +
> > > + interrupt-controller: true
> > > +
> > > + '#interrupt-cells':
> > > + const: 2
> > > + description:
> > > + The first cell is the input IRQ number, between 0 and 2, while the second
> > > + cell is the trigger type as defined in interrupt.txt in this directory.
> > > +
> > > + 'actions,ext-interrupts':
> > > + description: |
> > > + Contains the GIC SPI IRQ numbers mapped to the external interrupt
> > > + lines. They shall be specified sequentially from output 0 to 2.
> > > + $ref: /schemas/types.yaml#/definitions/uint32-array
> > > + minItems: 3
> > > + maxItems: 3
> >
> > Can't you use 'interrupts' here?
>
> This was actually my initial idea, but it might confuse the users since
> this is not following the parent controller IRQ specs, i.e. the trigger
> type is set internally by the SIRQ driver, it's not taken from DT.
Then what's the 2nd cell for?
> Please see the DTS sample bellow where both devices are on the same
> level and have GIC as interrupt parent. The 'interrupts' property
> in the sirq node looks incomplete now. That is why I decided to use
> a custom name for it, although I'm not sure it's the most relevant one,
> I am open to any other suggestion.
>
> i2c0: i2c@b0170000 {
> [...]
> interrupts = <GIC_SPI 25 IRQ_TYPE_LEVEL_HIGH>;
> [...]
> };
>
> sirq: interrupt-controller@b01b0200 {
> [...]
> interrupt-controller;
> #interrupt-cells = <2>;
> interrupts = <13>, /* SIRQ0 */
> <14>, /* SIRQ1 */
> <15>; /* SIRQ2 */
This isn't valid if the GIC is the parent as you have to have 3 cells
for each interrupt. Ultimately the GIC trigger type has to be
something. Is it fixed or passed thru? If the latter, just use 0
(IRQ_TYPE_NONE) if the GIC trigger mode is not fixed. Having some sort
of translation of the trigger is pretty common.
Rob
On Wed, Aug 26, 2020 at 04:48:38PM -0600, Rob Herring wrote:
> On Wed, Aug 26, 2020 at 3:42 PM Cristian Ciocaltea
> <[email protected]> wrote:
> >
> > Hi Rob,
> >
> > Thanks for the review!
> >
> > On Tue, Aug 25, 2020 at 04:09:13PM -0600, Rob Herring wrote:
> > > On Wed, Aug 19, 2020 at 07:37:56PM +0300, Cristian Ciocaltea wrote:
> > > > Actions Semi Owl SoCs SIRQ interrupt controller is found in S500, S700
> > > > and S900 SoCs and provides support for handling up to 3 external
> > > > interrupt lines.
> > > >
> > > > Signed-off-by: Cristian Ciocaltea <[email protected]>
> > > > ---
> > > > Changes in v5:
> > > > - Updated controller description statements both in the commit message
> > > > and the binding doc
> > > >
> > > > .../actions,owl-sirq.yaml | 68 +++++++++++++++++++
> > > > 1 file changed, 68 insertions(+)
> > > > create mode 100644 Documentation/devicetree/bindings/interrupt-controller/actions,owl-sirq.yaml
> > > >
> > > > diff --git a/Documentation/devicetree/bindings/interrupt-controller/actions,owl-sirq.yaml b/Documentation/devicetree/bindings/interrupt-controller/actions,owl-sirq.yaml
> > > > new file mode 100644
> > > > index 000000000000..cf9b7a514e4e
> > > > --- /dev/null
> > > > +++ b/Documentation/devicetree/bindings/interrupt-controller/actions,owl-sirq.yaml
> > > > @@ -0,0 +1,68 @@
> > > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > > > +%YAML 1.2
> > > > +---
> > > > +$id: http://devicetree.org/schemas/interrupt-controller/actions,owl-sirq.yaml#
> > > > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > > +
> > > > +title: Actions Semi Owl SoCs SIRQ interrupt controller
> > > > +
> > > > +maintainers:
> > > > + - Manivannan Sadhasivam <[email protected]>
> > > > + - Cristian Ciocaltea <[email protected]>
> > > > +
> > > > +description: |
> > > > + This interrupt controller is found in the Actions Semi Owl SoCs (S500, S700
> > > > + and S900) and provides support for handling up to 3 external interrupt lines.
> > > > +
> > > > +properties:
> > > > + compatible:
> > > > + oneOf:
> > > > + - items:
> > > > + - enum:
> > > > + - actions,s500-sirq
> > > > + - actions,s700-sirq
> > > > + - actions,s900-sirq
> > > > + - const: actions,owl-sirq
> > > > + - const: actions,owl-sirq
> > >
> > > This should be dropped. You should always have the SoC specific
> > > compatible.
> >
> > Sure, I will get rid of the 'owl-sirq' compatible.
> >
> > > > +
> > > > + reg:
> > > > + maxItems: 1
> > > > +
> > > > + interrupt-controller: true
> > > > +
> > > > + '#interrupt-cells':
> > > > + const: 2
> > > > + description:
> > > > + The first cell is the input IRQ number, between 0 and 2, while the second
> > > > + cell is the trigger type as defined in interrupt.txt in this directory.
> > > > +
> > > > + 'actions,ext-interrupts':
> > > > + description: |
> > > > + Contains the GIC SPI IRQ numbers mapped to the external interrupt
> > > > + lines. They shall be specified sequentially from output 0 to 2.
> > > > + $ref: /schemas/types.yaml#/definitions/uint32-array
> > > > + minItems: 3
> > > > + maxItems: 3
> > >
> > > Can't you use 'interrupts' here?
> >
> > This was actually my initial idea, but it might confuse the users since
> > this is not following the parent controller IRQ specs, i.e. the trigger
> > type is set internally by the SIRQ driver, it's not taken from DT.
>
> Then what's the 2nd cell for?
I should have added also a child device sample to make it more clear
how this is supposed to work:
&i2c0 {
atc260x: pmic@65 {
[...]
interrupt-parent = <&sirq>;
interrupts = <2 IRQ_TYPE_LEVEL_HIGH>;
};
};
The PMIC above uses the SIRQ2 pin of the SIRQ controller to trigger
interrupts, while the controller is responsible for proper translation
before sending to GIC, i.e. converting falling edge to rising edge signal
and active low to active high signal.
> > Please see the DTS sample bellow where both devices are on the same
> > level and have GIC as interrupt parent. The 'interrupts' property
> > in the sirq node looks incomplete now. That is why I decided to use
> > a custom name for it, although I'm not sure it's the most relevant one,
> > I am open to any other suggestion.
> >
> > i2c0: i2c@b0170000 {
> > [...]
> > interrupts = <GIC_SPI 25 IRQ_TYPE_LEVEL_HIGH>;
> > [...]
> > };
> >
> > sirq: interrupt-controller@b01b0200 {
> > [...]
> > interrupt-controller;
> > #interrupt-cells = <2>;
> > interrupts = <13>, /* SIRQ0 */
> > <14>, /* SIRQ1 */
> > <15>; /* SIRQ2 */
>
> This isn't valid if the GIC is the parent as you have to have 3 cells
> for each interrupt.
Right, that's the reason of replacing 'interrupts' with
'actions,ext-interrupts'.
> Ultimately the GIC trigger type has to be
> something. Is it fixed or passed thru? If the latter, just use 0
> (IRQ_TYPE_NONE) if the GIC trigger mode is not fixed. Having some sort
> of translation of the trigger is pretty common.
Yes, as explained above, the SIRQ controller performs indeed the
translation of the incoming signal. So if I understand correctly, your
suggestion would be to use the following inside the sirq node:
interrupts = <GIC_SPI 13 IRQ_TYPE_NONE>, /* SIRQ0 */
[...]
> Rob
Thanks,
Cristi
On 2020-08-27 11:06, Cristian Ciocaltea wrote:
> On Wed, Aug 26, 2020 at 04:48:38PM -0600, Rob Herring wrote:
>> On Wed, Aug 26, 2020 at 3:42 PM Cristian Ciocaltea
>> <[email protected]> wrote:
[...]
>> Ultimately the GIC trigger type has to be
>> something. Is it fixed or passed thru? If the latter, just use 0
>> (IRQ_TYPE_NONE) if the GIC trigger mode is not fixed. Having some sort
>> of translation of the trigger is pretty common.
>
> Yes, as explained above, the SIRQ controller performs indeed the
> translation of the incoming signal. So if I understand correctly, your
> suggestion would be to use the following inside the sirq node:
>
> interrupts = <GIC_SPI 13 IRQ_TYPE_NONE>, /* SIRQ0 */
> [...]
Please don't. If you are describing a GIC interrupt, use a
trigger that actually exists. Given that you have a 1:1
mapping between input and output, just encode the output
trigger that matches the input.
M.
--
Jazz is not dead. It just smells funny...
Hi Marc,
On Thu, Aug 27, 2020 at 11:35:06AM +0100, Marc Zyngier wrote:
> On 2020-08-27 11:06, Cristian Ciocaltea wrote:
> > On Wed, Aug 26, 2020 at 04:48:38PM -0600, Rob Herring wrote:
> > > On Wed, Aug 26, 2020 at 3:42 PM Cristian Ciocaltea
> > > <[email protected]> wrote:
>
> [...]
>
> > > Ultimately the GIC trigger type has to be
> > > something. Is it fixed or passed thru? If the latter, just use 0
> > > (IRQ_TYPE_NONE) if the GIC trigger mode is not fixed. Having some sort
> > > of translation of the trigger is pretty common.
> >
> > Yes, as explained above, the SIRQ controller performs indeed the
> > translation of the incoming signal. So if I understand correctly, your
> > suggestion would be to use the following inside the sirq node:
> >
> > interrupts = <GIC_SPI 13 IRQ_TYPE_NONE>, /* SIRQ0 */
> > [...]
>
> Please don't. If you are describing a GIC interrupt, use a
> trigger that actually exists. Given that you have a 1:1
> mapping between input and output, just encode the output
> trigger that matches the input.
Understood, the only remark here is that internally, the driver will
not use this information and instead will continue to rely on the input
to properly set the trigger type for the output.
The question is if the driver should also emit a warning (or error?)
when the trigger type supplied via DT doesn't match the expected value.
If yes, we should also clarify what the user is supposed to provide in
the controller node: the trigger type before the conversion (the input)
or the one after the conversion (the output).
> M.
> --
> Jazz is not dead. It just smells funny...
Thanks,
Cristi
Cristian,
On 2020-08-27 16:24, Cristian Ciocaltea wrote:
> Hi Marc,
>
> On Thu, Aug 27, 2020 at 11:35:06AM +0100, Marc Zyngier wrote:
>> On 2020-08-27 11:06, Cristian Ciocaltea wrote:
>> > On Wed, Aug 26, 2020 at 04:48:38PM -0600, Rob Herring wrote:
>> > > On Wed, Aug 26, 2020 at 3:42 PM Cristian Ciocaltea
>> > > <[email protected]> wrote:
>>
>> [...]
>>
>> > > Ultimately the GIC trigger type has to be
>> > > something. Is it fixed or passed thru? If the latter, just use 0
>> > > (IRQ_TYPE_NONE) if the GIC trigger mode is not fixed. Having some sort
>> > > of translation of the trigger is pretty common.
>> >
>> > Yes, as explained above, the SIRQ controller performs indeed the
>> > translation of the incoming signal. So if I understand correctly, your
>> > suggestion would be to use the following inside the sirq node:
>> >
>> > interrupts = <GIC_SPI 13 IRQ_TYPE_NONE>, /* SIRQ0 */
>> > [...]
>>
>> Please don't. If you are describing a GIC interrupt, use a
>> trigger that actually exists. Given that you have a 1:1
>> mapping between input and output, just encode the output
>> trigger that matches the input.
>
> Understood, the only remark here is that internally, the driver will
> not use this information and instead will continue to rely on the input
> to properly set the trigger type for the output.
It's fine. The binding has to be consistent on its own, but
doesn't dictate the way the driver does thing.
> The question is if the driver should also emit a warning (or error?)
> when the trigger type supplied via DT doesn't match the expected value.
Rob will tell you that the kernel isn't a validation tool for broken
DTs. Shout if you want, but you are allowed to simply ignore the
output trigger for example
> If yes, we should also clarify what the user is supposed to provide in
> the controller node: the trigger type before the conversion (the input)
> or the one after the conversion (the output).
The output of a SIRQ should be compatible with the GIC input it is
attached to. You can have:
device (LEVEL_LOW) -> SIRQ (LEVEL_HIGH) -> GIC
but you can't have:
device (LEVEL_LOW) -> SIRQ (EDGE_RISING) -> GIC
because that's not an acceptable transformation for the SIRQ,
nor can you have:
device (EDGE_FALLING) -> SIRQ (EDGE_FALLING) -> GIC
because EDGE_FALLING isn't a valid input for the GIC.
In both of the invalid cases, you would be free to apply
which ever transformation actually makes sense, and shout
at the user if you want to help them debugging their turf.
The later part is definitely optional.
Hope this helps,
M.
--
Jazz is not dead. It just smells funny...
On Thu, Aug 27, 2020 at 04:42:04PM +0100, Marc Zyngier wrote:
> Cristian,
>
> On 2020-08-27 16:24, Cristian Ciocaltea wrote:
> > Hi Marc,
> >
> > On Thu, Aug 27, 2020 at 11:35:06AM +0100, Marc Zyngier wrote:
> > > On 2020-08-27 11:06, Cristian Ciocaltea wrote:
> > > > On Wed, Aug 26, 2020 at 04:48:38PM -0600, Rob Herring wrote:
> > > > > On Wed, Aug 26, 2020 at 3:42 PM Cristian Ciocaltea
> > > > > <[email protected]> wrote:
> > >
> > > [...]
> > >
> > > > > Ultimately the GIC trigger type has to be
> > > > > something. Is it fixed or passed thru? If the latter, just use 0
> > > > > (IRQ_TYPE_NONE) if the GIC trigger mode is not fixed. Having some sort
> > > > > of translation of the trigger is pretty common.
> > > >
> > > > Yes, as explained above, the SIRQ controller performs indeed the
> > > > translation of the incoming signal. So if I understand correctly, your
> > > > suggestion would be to use the following inside the sirq node:
> > > >
> > > > interrupts = <GIC_SPI 13 IRQ_TYPE_NONE>, /* SIRQ0 */
> > > > [...]
> > >
> > > Please don't. If you are describing a GIC interrupt, use a
> > > trigger that actually exists. Given that you have a 1:1
> > > mapping between input and output, just encode the output
> > > trigger that matches the input.
> >
> > Understood, the only remark here is that internally, the driver will
> > not use this information and instead will continue to rely on the input
> > to properly set the trigger type for the output.
>
> It's fine. The binding has to be consistent on its own, but
> doesn't dictate the way the driver does thing.
>
> > The question is if the driver should also emit a warning (or error?)
> > when the trigger type supplied via DT doesn't match the expected value.
>
> Rob will tell you that the kernel isn't a validation tool for broken
> DTs. Shout if you want, but you are allowed to simply ignore the
> output trigger for example
>
> > If yes, we should also clarify what the user is supposed to provide in
> > the controller node: the trigger type before the conversion (the input)
> > or the one after the conversion (the output).
>
> The output of a SIRQ should be compatible with the GIC input it is
> attached to. You can have:
>
> device (LEVEL_LOW) -> SIRQ (LEVEL_HIGH) -> GIC
>
> but you can't have:
>
> device (LEVEL_LOW) -> SIRQ (EDGE_RISING) -> GIC
>
> because that's not an acceptable transformation for the SIRQ,
> nor can you have:
>
> device (EDGE_FALLING) -> SIRQ (EDGE_FALLING) -> GIC
>
> because EDGE_FALLING isn't a valid input for the GIC.
>
> In both of the invalid cases, you would be free to apply
> which ever transformation actually makes sense, and shout
> at the user if you want to help them debugging their turf.
> The later part is definitely optional.
>
> Hope this helps,
This certainly helps a lot, now I have a clear understanding of what is
to be done next.
> M.
> --
> Jazz is not dead. It just smells funny...
Many thanks for the detailed explanations,
Cristi