[PATCH v2 0/3] Add Actions Semi Owl family sirq support

[PATCH v2 0/3] Add Actions Semi Owl family sirq support

[Parthiban Nallathambi]

This patch series add support for external interrupt controller
in Actions Semi Owl famil of SoC's (S500, S700 and S900). Actions
provides support for external interrupt controller to be connected
with it's SoC's using 3 SIRQ pins.

Each line can be configures independently, i.e 3 independent external
interrupt controller can be connected and managed parallely.

Device tree node is created only for S700 after testing it in Cubieboard7.

Changelog in v2:
- 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

Thanks,
Parthiban
Saravanan

Parthiban Nallathambi (3):
  dt-bindings: interrupt-controller: Actions external interrupt
    controller
  drivers/irqchip: Add Actions external interrupts support
  arm64: dts: actions: Add sirq node for Actions Semi S700

 .../interrupt-controller/actions,owl-sirq.txt      |  46 ++++
 arch/arm64/boot/dts/actions/s700.dtsi              |   9 +
 drivers/irqchip/Makefile                           |   1 +
 drivers/irqchip/irq-owl-sirq.c                     | 305 +++++++++++++++++++++
 4 files changed, 361 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/interrupt-controller/actions,owl-sirq.txt
 create mode 100644 drivers/irqchip/irq-owl-sirq.c

-- 
2.14.4

[PATCH v2 1/3] dt-bindings: interrupt-controller: Actions external interrupt controller

[Parthiban Nallathambi]

Actions Semi OWL family SoC's provides support for external interrupt
controller to be connected and controlled using SIRQ pins. S500, S700
and S900 provides 3 SIRQ lines and works independently for 3 external
interrupt controllers.

Signed-off-by: Parthiban Nallathambi <pn@denx.de>
Signed-off-by: Saravanan Sekar <sravanhome@gmail.com>
---
 .../interrupt-controller/actions,owl-sirq.txt      | 46 ++++++++++++++++++++++
 1 file changed, 46 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/interrupt-controller/actions,owl-sirq.txt

diff --git a/Documentation/devicetree/bindings/interrupt-controller/actions,owl-sirq.txt b/Documentation/devicetree/bindings/interrupt-controller/actions,owl-sirq.txt
new file mode 100644
index 000000000000..4b8437751331
--- /dev/null
+++ b/Documentation/devicetree/bindings/interrupt-controller/actions,owl-sirq.txt
@@ -0,0 +1,46 @@
+Actions Semi Owl SoCs SIRQ interrupt controller
+
+S500, S700 and S900 SoC's from Actions provides 3 SPI's from GIC,
+in which external interrupt controller can be connected. 3 SPI's
+45, 46, 47 from GIC are directly exposed as SIRQ. It has
+the following properties:
+
+- inputs three interrupt signal from external interrupt controller
+
+Required properties:
+
+- compatible: should be "actions,owl-sirq"
+- reg: physical base address of the controller and length of memory mapped.
+- interrupt-controller: identifies the node as an interrupt controller
+- #interrupt-cells: specifies the number of cells needed to encode an interrupt
+  source, should be 2.
+- actions,sirq-shared-reg: Applicable for S500 and S700 where SIRQ register
+  details are maintained at same offset/register.
+- actions,sirq-offset: register offset for SIRQ interrupts. When registers are
+  shared, all the three offsets will be same (S500 and S700).
+- actions,sirq-clk-sel: external interrupt controller can be either
+  connected to 32Khz or 24Mhz external/internal clock. This needs
+  to be configured for per SIRQ line. Failing defaults to 32Khz clock.
+
+Example for S900:
+
+sirq: interrupt-controller@e01b0000 {
+	compatible = "actions,owl-sirq";
+	reg = <0 0xe01b0000 0 0x1000>;
+	interrupt-controller;
+	#interrupt-cells = <2>;
+	actions,sirq-clk-sel = <0 0 0>;
+	actions,sirq-offset = <0x200 0x528 0x52c>;
+};
+
+Example for S500 and S700:
+
+sirq: interrupt-controller@e01b0000 {
+	compatible = "actions,owl-sirq";
+	reg = <0 0xe01b0000 0 0x1000>;
+	interrupt-controller;
+	#interrupt-cells = <2>;
+	actions,sirq-shared-reg;
+	actions,sirq-clk-sel = <0 0 0>;
+	actions,sirq-offset = <0x200 0x200 0x200>;
+};
-- 
2.14.4

[PATCH v2 2/3] drivers/irqchip: Add Actions external interrupts support

[Parthiban Nallathambi]

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@denx.de>
Signed-off-by: Saravanan Sekar <sravanhome@gmail.com>
---
 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@actions-semi.com>
+ *
+ * Author: Parthiban Nallathambi <pn@denx.de>
+ * Author: Saravanan Sekar <sravanhome@gmail.com>
+ */
+
+#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
+#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.
+	 */
+	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;
+};
+
+static struct owl_sirq_chip_data *sirq_data;
+
+static unsigned int sirq_read_extctl(struct irq_data *data)
+{
+	struct owl_sirq_chip_data *chip_data = data->chip_data;
+	unsigned int val;
+
+	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)
+{
+	struct owl_sirq_chip_data *chip_data = data->chip_data;
+	unsigned int val;
+
+	if (chip_data->shared_reg) {
+		val = readl_relaxed(chip_data->base +
+				get_sirq_offset(data->hwirq));
+		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));
+}
+
+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);
+	}
+	irq_chip_ack_parent(data);
+}
+
+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;
+	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],
+		.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,
+};
+
+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);
+}
+
+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);
-- 
2.14.4

[PATCH v2 3/3] arm64: dts: actions: Add sirq node for Actions Semi S700

[Parthiban Nallathambi]

Add sirq node for Actions Semi S700 SoC with 3 SIRQ pins support,
in which external interrupt controllers can be connected.

Example:
atc260x: atc2603c@65 {
	interrupt-parent = <&sirq>;
	interrupts = <2 IRQ_TYPE_LEVEL_HIGH>;
};

Signed-off-by: Parthiban Nallathambi <pn@denx.de>
Signed-off-by: Saravanan Sekar <sravanhome@gmail.com>
---
 arch/arm64/boot/dts/actions/s700.dtsi | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/arch/arm64/boot/dts/actions/s700.dtsi b/arch/arm64/boot/dts/actions/s700.dtsi
index 66dd5309f0a2..c5aef5ac7f46 100644
--- a/arch/arm64/boot/dts/actions/s700.dtsi
+++ b/arch/arm64/boot/dts/actions/s700.dtsi
@@ -153,6 +153,15 @@
 			status = "disabled";
 		};
 
+		sirq: interrupt-controller@e01b0000 {
+			compatible = "actions,owl-sirq";
+			reg = <0 0xe01b0000 0 0x1000>;
+			interrupt-controller;
+			#interrupt-cells = <2>;
+			actions,sirq-shared-reg;
+			actions,sirq-offset = <0x200 0x200 0x200>;
+		};
+
 		sps: power-controller@e01b0100 {
 			compatible = "actions,s700-sps";
 			reg = <0x0 0xe01b0100 0x0 0x100>;
-- 
2.14.4

Re: [PATCH v2 1/3] dt-bindings: interrupt-controller: Actions external interrupt controller

[Manivannan Sadhasivam]

Hi Parthiban,

On Sun, Aug 12, 2018 at 02:22:13PM +0200, Parthiban Nallathambi wrote:
> Actions Semi OWL family SoC's provides support for external interrupt
> controller to be connected and controlled using SIRQ pins. S500, S700
> and S900 provides 3 SIRQ lines and works independently for 3 external
> interrupt controllers.
> 
> Signed-off-by: Parthiban Nallathambi <pn@denx.de>
> Signed-off-by: Saravanan Sekar <sravanhome@gmail.com>
> ---
>  .../interrupt-controller/actions,owl-sirq.txt      | 46 ++++++++++++++++++++++
>  1 file changed, 46 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/interrupt-controller/actions,owl-sirq.txt
> 
> diff --git a/Documentation/devicetree/bindings/interrupt-controller/actions,owl-sirq.txt b/Documentation/devicetree/bindings/interrupt-controller/actions,owl-sirq.txt
> new file mode 100644
> index 000000000000..4b8437751331
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/interrupt-controller/actions,owl-sirq.txt
> @@ -0,0 +1,46 @@
> +Actions Semi Owl SoCs SIRQ interrupt controller
> +
> +S500, S700 and S900 SoC's from Actions provides 3 SPI's from GIC,
> +in which external interrupt controller can be connected. 3 SPI's
> +45, 46, 47 from GIC are directly exposed as SIRQ. It has
> +the following properties:

We should really document the driver here. What it does? and how the
hierarchy is handled with GIC? etc...

> +
> +- inputs three interrupt signal from external interrupt controller
> +
> +Required properties:
> +
> +- compatible: should be "actions,owl-sirq"
> +- reg: physical base address of the controller and length of memory mapped.

...length of memory mapped region?

> +- interrupt-controller: identifies the node as an interrupt controller
> +- #interrupt-cells: specifies the number of cells needed to encode an interrupt
> +  source, should be 2.
> +- actions,sirq-shared-reg: Applicable for S500 and S700 where SIRQ register
> +  details are maintained at same offset/register.
> +- actions,sirq-offset: register offset for SIRQ interrupts. When registers are
> +  shared, all the three offsets will be same (S500 and S700).
> +- actions,sirq-clk-sel: external interrupt controller can be either
> +  connected to 32Khz or 24Mhz external/internal clock. This needs

Hertz should be specified as Hz.

> +  to be configured for per SIRQ line. Failing defaults to 32Khz clock.

What value needs to be specified for selecting 24MHz clock? You should
mention the available options this property supports.

> +
> +Example for S900:
> +
> +sirq: interrupt-controller@e01b0000 {
> +	compatible = "actions,owl-sirq";
> +	reg = <0 0xe01b0000 0 0x1000>;

could be: reg = <0x0 0xe01b0000 0x0 0x1000>;

> +	interrupt-controller;
> +	#interrupt-cells = <2>;
> +	actions,sirq-clk-sel = <0 0 0>;
> +	actions,sirq-offset = <0x200 0x528 0x52c>;
> +};
> +
> +Example for S500 and S700:
> +
> +sirq: interrupt-controller@e01b0000 {
> +	compatible = "actions,owl-sirq";
> +	reg = <0 0xe01b0000 0 0x1000>;

For S500, reg base is 0xb01b0000.

Thanks
Mani

> +	interrupt-controller;
> +	#interrupt-cells = <2>;
> +	actions,sirq-shared-reg;
> +	actions,sirq-clk-sel = <0 0 0>;
> +	actions,sirq-offset = <0x200 0x200 0x200>;
> +};
> -- 
> 2.14.4
> 

Re: [PATCH v2 1/3] dt-bindings: interrupt-controller: Actions external interrupt controller

[Marc Zyngier]

On 13/08/18 05:34, Manivannan Sadhasivam wrote:
> Hi Parthiban,
> 
> On Sun, Aug 12, 2018 at 02:22:13PM +0200, Parthiban Nallathambi wrote:
>> Actions Semi OWL family SoC's provides support for external interrupt
>> controller to be connected and controlled using SIRQ pins. S500, S700
>> and S900 provides 3 SIRQ lines and works independently for 3 external
>> interrupt controllers.
>>
>> Signed-off-by: Parthiban Nallathambi <pn@denx.de>
>> Signed-off-by: Saravanan Sekar <sravanhome@gmail.com>
>> ---
>>  .../interrupt-controller/actions,owl-sirq.txt      | 46 ++++++++++++++++++++++
>>  1 file changed, 46 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/interrupt-controller/actions,owl-sirq.txt
>>
>> diff --git a/Documentation/devicetree/bindings/interrupt-controller/actions,owl-sirq.txt b/Documentation/devicetree/bindings/interrupt-controller/actions,owl-sirq.txt
>> new file mode 100644
>> index 000000000000..4b8437751331
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/interrupt-controller/actions,owl-sirq.txt
>> @@ -0,0 +1,46 @@
>> +Actions Semi Owl SoCs SIRQ interrupt controller
>> +
>> +S500, S700 and S900 SoC's from Actions provides 3 SPI's from GIC,
>> +in which external interrupt controller can be connected. 3 SPI's
>> +45, 46, 47 from GIC are directly exposed as SIRQ. It has
>> +the following properties:
> 
> We should really document the driver here. What it does? and how the
> hierarchy is handled with GIC? etc...

Absolutely not. This should document the binding, irrespective of the
operating system. The word "driver" is completely out of place here.

Thanks,

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

Re: [PATCH v2 1/3] dt-bindings: interrupt-controller: Actions external interrupt controller

[Manivannan Sadhasivam]

On Mon, Aug 13, 2018 at 08:51:54AM +0100, Marc Zyngier wrote:
> On 13/08/18 05:34, Manivannan Sadhasivam wrote:
> > Hi Parthiban,
> > 
> > On Sun, Aug 12, 2018 at 02:22:13PM +0200, Parthiban Nallathambi wrote:
> >> Actions Semi OWL family SoC's provides support for external interrupt
> >> controller to be connected and controlled using SIRQ pins. S500, S700
> >> and S900 provides 3 SIRQ lines and works independently for 3 external
> >> interrupt controllers.
> >>
> >> Signed-off-by: Parthiban Nallathambi <pn@denx.de>
> >> Signed-off-by: Saravanan Sekar <sravanhome@gmail.com>
> >> ---
> >>  .../interrupt-controller/actions,owl-sirq.txt      | 46 ++++++++++++++++++++++
> >>  1 file changed, 46 insertions(+)
> >>  create mode 100644 Documentation/devicetree/bindings/interrupt-controller/actions,owl-sirq.txt
> >>
> >> diff --git a/Documentation/devicetree/bindings/interrupt-controller/actions,owl-sirq.txt b/Documentation/devicetree/bindings/interrupt-controller/actions,owl-sirq.txt
> >> new file mode 100644
> >> index 000000000000..4b8437751331
> >> --- /dev/null
> >> +++ b/Documentation/devicetree/bindings/interrupt-controller/actions,owl-sirq.txt
> >> @@ -0,0 +1,46 @@
> >> +Actions Semi Owl SoCs SIRQ interrupt controller
> >> +
> >> +S500, S700 and S900 SoC's from Actions provides 3 SPI's from GIC,
> >> +in which external interrupt controller can be connected. 3 SPI's
> >> +45, 46, 47 from GIC are directly exposed as SIRQ. It has
> >> +the following properties:
> > 
> > We should really document the driver here. What it does? and how the
> > hierarchy is handled with GIC? etc...
> 
> Absolutely not. This should document the binding, irrespective of the
> operating system. The word "driver" is completely out of place here.
>

Oops, my bad. I meant to say that we should add it as a description in the
commit message.

Thanks,
Mani

> Thanks,
> 
> 	M.
> -- 
> Jazz is not dead. It just smells funny...

Re: [PATCH v2 1/3] dt-bindings: interrupt-controller: Actions external interrupt controller

[Manivannan Sadhasivam]

On Mon, Aug 13, 2018 at 02:45:47PM +0530, Manivannan Sadhasivam wrote:
> On Mon, Aug 13, 2018 at 08:51:54AM +0100, Marc Zyngier wrote:
> > On 13/08/18 05:34, Manivannan Sadhasivam wrote:
> > > Hi Parthiban,
> > > 
> > > On Sun, Aug 12, 2018 at 02:22:13PM +0200, Parthiban Nallathambi wrote:
> > >> Actions Semi OWL family SoC's provides support for external interrupt
> > >> controller to be connected and controlled using SIRQ pins. S500, S700
> > >> and S900 provides 3 SIRQ lines and works independently for 3 external
> > >> interrupt controllers.
> > >>
> > >> Signed-off-by: Parthiban Nallathambi <pn@denx.de>
> > >> Signed-off-by: Saravanan Sekar <sravanhome@gmail.com>
> > >> ---
> > >>  .../interrupt-controller/actions,owl-sirq.txt      | 46 ++++++++++++++++++++++
> > >>  1 file changed, 46 insertions(+)
> > >>  create mode 100644 Documentation/devicetree/bindings/interrupt-controller/actions,owl-sirq.txt
> > >>
> > >> diff --git a/Documentation/devicetree/bindings/interrupt-controller/actions,owl-sirq.txt b/Documentation/devicetree/bindings/interrupt-controller/actions,owl-sirq.txt
> > >> new file mode 100644
> > >> index 000000000000..4b8437751331
> > >> --- /dev/null
> > >> +++ b/Documentation/devicetree/bindings/interrupt-controller/actions,owl-sirq.txt
> > >> @@ -0,0 +1,46 @@
> > >> +Actions Semi Owl SoCs SIRQ interrupt controller
> > >> +
> > >> +S500, S700 and S900 SoC's from Actions provides 3 SPI's from GIC,
> > >> +in which external interrupt controller can be connected. 3 SPI's
> > >> +45, 46, 47 from GIC are directly exposed as SIRQ. It has
> > >> +the following properties:
> > > 
> > > We should really document the driver here. What it does? and how the
> > > hierarchy is handled with GIC? etc...
> > 
> > Absolutely not. This should document the binding, irrespective of the
> > operating system. The word "driver" is completely out of place here.
> >
> 
> Oops, my bad. I meant to say that we should add it as a description in the
> commit message.
>

...commit message of the driver patch (to be more explicit)
 
> Thanks,
> Mani
> 
> > Thanks,
> > 
> > 	M.
> > -- 
> > Jazz is not dead. It just smells funny...

Re: [PATCH v2 2/3] drivers/irqchip: Add Actions external interrupts support

[Marc Zyngier]

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@denx.de>
> Signed-off-by: Saravanan Sekar <sravanhome@gmail.com>
> ---
>  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@actions-semi.com>
> + *
> + * Author: Parthiban Nallathambi <pn@denx.de>
> + * Author: Saravanan Sekar <sravanhome@gmail.com>
> + */
> +
> +#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?

> +#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.
 */

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

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

> +{
> +	struct owl_sirq_chip_data *chip_data = data->chip_data;
> +	unsigned int val;

u32;

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

> +{
> +	struct owl_sirq_chip_data *chip_data = data->chip_data;
> +	unsigned int val;

u32;

> +
> +	if (chip_data->shared_reg) {
> +		val = readl_relaxed(chip_data->base +
> +				get_sirq_offset(data->hwirq));

Single line, please.

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

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

> +	}
> +	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?

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

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

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

> +};
> +
> +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?

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

Thanks,

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

Re: [PATCH v2 1/3] dt-bindings: interrupt-controller: Actions external interrupt controller

[Rob Herring]

On Sun, Aug 12, 2018 at 02:22:13PM +0200, Parthiban Nallathambi wrote:
> Actions Semi OWL family SoC's provides support for external interrupt
> controller to be connected and controlled using SIRQ pins. S500, S700
> and S900 provides 3 SIRQ lines and works independently for 3 external
> interrupt controllers.
> 
> Signed-off-by: Parthiban Nallathambi <pn@denx.de>
> Signed-off-by: Saravanan Sekar <sravanhome@gmail.com>
> ---
>  .../interrupt-controller/actions,owl-sirq.txt      | 46 ++++++++++++++++++++++
>  1 file changed, 46 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/interrupt-controller/actions,owl-sirq.txt
> 
> diff --git a/Documentation/devicetree/bindings/interrupt-controller/actions,owl-sirq.txt b/Documentation/devicetree/bindings/interrupt-controller/actions,owl-sirq.txt
> new file mode 100644
> index 000000000000..4b8437751331
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/interrupt-controller/actions,owl-sirq.txt
> @@ -0,0 +1,46 @@
> +Actions Semi Owl SoCs SIRQ interrupt controller
> +
> +S500, S700 and S900 SoC's from Actions provides 3 SPI's from GIC,
> +in which external interrupt controller can be connected. 3 SPI's
> +45, 46, 47 from GIC are directly exposed as SIRQ. It has
> +the following properties:
> +
> +- inputs three interrupt signal from external interrupt controller
> +
> +Required properties:
> +
> +- compatible: should be "actions,owl-sirq"
> +- reg: physical base address of the controller and length of memory mapped.
> +- interrupt-controller: identifies the node as an interrupt controller
> +- #interrupt-cells: specifies the number of cells needed to encode an interrupt
> +  source, should be 2.

> +- actions,sirq-shared-reg: Applicable for S500 and S700 where SIRQ register
> +  details are maintained at same offset/register.
> +- actions,sirq-offset: register offset for SIRQ interrupts. When registers are
> +  shared, all the three offsets will be same (S500 and S700).

You should have more specific compatible strings if there are 
differences and these settings can be implied by them.

> +- actions,sirq-clk-sel: external interrupt controller can be either
> +  connected to 32Khz or 24Mhz external/internal clock. This needs
> +  to be configured for per SIRQ line. Failing defaults to 32Khz clock.

What are the valid values?

> +
> +Example for S900:
> +
> +sirq: interrupt-controller@e01b0000 {
> +	compatible = "actions,owl-sirq";
> +	reg = <0 0xe01b0000 0 0x1000>;
> +	interrupt-controller;
> +	#interrupt-cells = <2>;
> +	actions,sirq-clk-sel = <0 0 0>;

If 0 is 32khz, then having this is pointless. But I can't tell what the 
values correspond to.

> +	actions,sirq-offset = <0x200 0x528 0x52c>;
> +};
> +
> +Example for S500 and S700:
> +
> +sirq: interrupt-controller@e01b0000 {
> +	compatible = "actions,owl-sirq";
> +	reg = <0 0xe01b0000 0 0x1000>;
> +	interrupt-controller;
> +	#interrupt-cells = <2>;
> +	actions,sirq-shared-reg;
> +	actions,sirq-clk-sel = <0 0 0>;
> +	actions,sirq-offset = <0x200 0x200 0x200>;
> +};
> -- 
> 2.14.4
> 

Re: [PATCH v2 2/3] drivers/irqchip: Add Actions external interrupts support

[Parthiban Nallathambi]

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@denx.de>
>> Signed-off-by: Saravanan Sekar <sravanhome@gmail.com>
>> ---
>>  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@actions-semi.com>
>> + *
>> + * Author: Parthiban Nallathambi <pn@denx.de>
>> + * Author: Saravanan Sekar <sravanhome@gmail.com>
>> + */
>> +
>> +#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

Re: [PATCH v2 2/3] drivers/irqchip: Add Actions external interrupts support

[Parthiban Nallathambi]

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@denx.de>
>>> Signed-off-by: Saravanan Sekar <sravanhome@gmail.com>
>>> ---
>>>   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@actions-semi.com>
>>> + *
>>> + * Author: Parthiban Nallathambi <pn@denx.de>
>>> + * Author: Saravanan Sekar <sravanhome@gmail.com>
>>> + */
>>> +
>>> +#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

Re: [PATCH v2 2/3] drivers/irqchip: Add Actions external interrupts support

[Parthiban Nallathambi]

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@denx.de>
>>>> Signed-off-by: Saravanan Sekar <sravanhome@gmail.com>
>>>> ---
>>>>   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@actions-semi.com>
>>>> + *
>>>> + * Author: Parthiban Nallathambi <pn@denx.de>
>>>> + * Author: Saravanan Sekar <sravanhome@gmail.com>
>>>> + */
>>>> +
>>>> +#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@denx.de

Re: [PATCH v2 2/3] drivers/irqchip: Add Actions external interrupts support

[Marc Zyngier]

On 26/08/18 16:20, 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@denx.de>
>>> Signed-off-by: Saravanan Sekar <sravanhome@gmail.com>
>>> ---
>>>  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@actions-semi.com>
>>> + *
>>> + * Author: Parthiban Nallathambi <pn@denx.de>
>>> + * Author: Saravanan Sekar <sravanhome@gmail.com>
>>> + */
>>> +
>>> +#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.

This is normally shown as a property from DT, and is relative to the
parent irqchip. And I don't understand what you mean by "offset into the
register". The only use of this is to allocate the corresponding GIC
interrupt, and this definitely shouldn't be harcoded.

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

If you don't have any way to test it, is it worth it to have that code
in? I'd prefer you add code that actually works, even if that's for a
subset of the capability of the HW, rather than add code that cannot be
exercised.

> 
>>
>>> +}
>>> +
>>> +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?

I think this should be entirely hidden from the interrupt controller,
and set by firmware or by the platform clock setup.

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

I must say I've lost track of this driver a while ago. Can you please
send whatever you have come up with, and we'll take it from there.

Thanks,

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

Re: [PATCH v2 2/3] drivers/irqchip: Add Actions external interrupts support

[Parthiban Nallathambi]

On 11/8/18 6:03 PM, Marc Zyngier wrote:
> On 26/08/18 16:20, 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@denx.de>
>>>> Signed-off-by: Saravanan Sekar <sravanhome@gmail.com>
>>>> ---
>>>>   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@actions-semi.com>
>>>> + *
>>>> + * Author: Parthiban Nallathambi <pn@denx.de>
>>>> + * Author: Saravanan Sekar <sravanhome@gmail.com>
>>>> + */
>>>> +
>>>> +#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.
> 
> This is normally shown as a property from DT, and is relative to the
> parent irqchip. And I don't understand what you mean by "offset into the
> register". The only use of this is to allocate the corresponding GIC

We have two SoC's (s500, s700) with shared external interrupt control
register and one (s900) with dedicated register for each external
interrupt line. So the DT property "actions,sirq-offset" was introduced
to access the register.

In case of s500, s700 when it's shared, the idea is to use the "hwirq"
variable value to offset into the control register INTC_EXTCTL. Even if
3 cell GIC value is directly used like

interrupts = <GIC_SPI 2 IRQ_TYPE_LEVEL_HIGH>;

then hwirq - 13 is needed internally everywhere in this driver.

In short, this value is defined inside driver for the ease of referring
the offset with the register.

Yes, it is possible to change the driver logic and use 3 cell interrupts
from DT.

> interrupt, and this definitely shouldn't be harcoded.
> 
>>
>> 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.
> 
> If you don't have any way to test it, is it worth it to have that code
> in? I'd prefer you add code that actually works, even if that's for a
> subset of the capability of the HW, rather than add code that cannot be
> exercised.

Ok, it wasn't case now. I have the hardware connected on these lines and
tested ok with the implementation moved to .irq.eoi.

> 
>>
>>>
>>>> +}
>>>> +
>>>> +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?
> 
> I think this should be entirely hidden from the interrupt controller,
> and set by firmware or by the platform clock setup.

Agreed!

> 
>>
>>>
>>>> +
>>>> +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!
> 
> I must say I've lost track of this driver a while ago. Can you please
> send whatever you have come up with, and we'll take it from there.

Sure, there is one thing changed, tested the eoi part with the PMIC
connected on this interrupt line and it works.

Should I send v3 using 3 interrupt cells and re-define the usage of
hwirq to offsetting?

Thanks,
Parthi

> 
> Thanks,
> 
> 	M.
> 

Re: [PATCH v2 1/3] dt-bindings: interrupt-controller: Actions external interrupt controller

[Parthiban Nallathambi]

On 8/13/18 6:34 AM, Manivannan Sadhasivam wrote:
> Hi Parthiban,
> 
> On Sun, Aug 12, 2018 at 02:22:13PM +0200, Parthiban Nallathambi wrote:
>> Actions Semi OWL family SoC's provides support for external interrupt
>> controller to be connected and controlled using SIRQ pins. S500, S700
>> and S900 provides 3 SIRQ lines and works independently for 3 external
>> interrupt controllers.
>>
>> Signed-off-by: Parthiban Nallathambi <pn@denx.de>
>> Signed-off-by: Saravanan Sekar <sravanhome@gmail.com>
>> ---
>>   .../interrupt-controller/actions,owl-sirq.txt      | 46 ++++++++++++++++++++++
>>   1 file changed, 46 insertions(+)
>>   create mode 100644 Documentation/devicetree/bindings/interrupt-controller/actions,owl-sirq.txt
>>
>> diff --git a/Documentation/devicetree/bindings/interrupt-controller/actions,owl-sirq.txt b/Documentation/devicetree/bindings/interrupt-controller/actions,owl-sirq.txt
>> new file mode 100644
>> index 000000000000..4b8437751331
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/interrupt-controller/actions,owl-sirq.txt
>> @@ -0,0 +1,46 @@
>> +Actions Semi Owl SoCs SIRQ interrupt controller
>> +
>> +S500, S700 and S900 SoC's from Actions provides 3 SPI's from GIC,
>> +in which external interrupt controller can be connected. 3 SPI's
>> +45, 46, 47 from GIC are directly exposed as SIRQ. It has
>> +the following properties:
> 
> We should really document the driver here. What it does? and how the
> hierarchy is handled with GIC? etc...
> 
>> +
>> +- inputs three interrupt signal from external interrupt controller
>> +
>> +Required properties:
>> +
>> +- compatible: should be "actions,owl-sirq"
>> +- reg: physical base address of the controller and length of memory mapped.
> 
> ...length of memory mapped region?

Yes it is.

> 
>> +- interrupt-controller: identifies the node as an interrupt controller
>> +- #interrupt-cells: specifies the number of cells needed to encode an interrupt
>> +  source, should be 2.
>> +- actions,sirq-shared-reg: Applicable for S500 and S700 where SIRQ register
>> +  details are maintained at same offset/register.
>> +- actions,sirq-offset: register offset for SIRQ interrupts. When registers are
>> +  shared, all the three offsets will be same (S500 and S700).
>> +- actions,sirq-clk-sel: external interrupt controller can be either
>> +  connected to 32Khz or 24Mhz external/internal clock. This needs
> 
> Hertz should be specified as Hz.
> 
>> +  to be configured for per SIRQ line. Failing defaults to 32Khz clock.
> 
> What value needs to be specified for selecting 24MHz clock? You should
> mention the available options this property supports.

Ok, this property will be removed here and leave to default 24MHz for
all the SoC's.

> 
>> +
>> +Example for S900:
>> +
>> +sirq: interrupt-controller@e01b0000 {
>> +	compatible = "actions,owl-sirq";
>> +	reg = <0 0xe01b0000 0 0x1000>;
> 
> could be: reg = <0x0 0xe01b0000 0x0 0x1000>;

Agreed!

> 
>> +	interrupt-controller;
>> +	#interrupt-cells = <2>;
>> +	actions,sirq-clk-sel = <0 0 0>;
>> +	actions,sirq-offset = <0x200 0x528 0x52c>;
>> +};
>> +
>> +Example for S500 and S700:
>> +
>> +sirq: interrupt-controller@e01b0000 {
>> +	compatible = "actions,owl-sirq";
>> +	reg = <0 0xe01b0000 0 0x1000>;
> 
> For S500, reg base is 0xb01b0000.

Yes, agreed!

Thanks,
Parthi

> 
> Thanks
> Mani
> 
>> +	interrupt-controller;
>> +	#interrupt-cells = <2>;
>> +	actions,sirq-shared-reg;
>> +	actions,sirq-clk-sel = <0 0 0>;
>> +	actions,sirq-offset = <0x200 0x200 0x200>;
>> +};
>> -- 
>> 2.14.4
>>
>

Re: [PATCH v2 1/3] dt-bindings: interrupt-controller: Actions external interrupt controller

[Parthiban Nallathambi]

On 8/13/18 9:44 PM, Rob Herring wrote:
> On Sun, Aug 12, 2018 at 02:22:13PM +0200, Parthiban Nallathambi wrote:
>> Actions Semi OWL family SoC's provides support for external interrupt
>> controller to be connected and controlled using SIRQ pins. S500, S700
>> and S900 provides 3 SIRQ lines and works independently for 3 external
>> interrupt controllers.
>>
>> Signed-off-by: Parthiban Nallathambi <pn@denx.de>
>> Signed-off-by: Saravanan Sekar <sravanhome@gmail.com>
>> ---
>>   .../interrupt-controller/actions,owl-sirq.txt      | 46 ++++++++++++++++++++++
>>   1 file changed, 46 insertions(+)
>>   create mode 100644 Documentation/devicetree/bindings/interrupt-controller/actions,owl-sirq.txt
>>
>> diff --git a/Documentation/devicetree/bindings/interrupt-controller/actions,owl-sirq.txt b/Documentation/devicetree/bindings/interrupt-controller/actions,owl-sirq.txt
>> new file mode 100644
>> index 000000000000..4b8437751331
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/interrupt-controller/actions,owl-sirq.txt
>> @@ -0,0 +1,46 @@
>> +Actions Semi Owl SoCs SIRQ interrupt controller
>> +
>> +S500, S700 and S900 SoC's from Actions provides 3 SPI's from GIC,
>> +in which external interrupt controller can be connected. 3 SPI's
>> +45, 46, 47 from GIC are directly exposed as SIRQ. It has
>> +the following properties:
>> +
>> +- inputs three interrupt signal from external interrupt controller
>> +
>> +Required properties:
>> +
>> +- compatible: should be "actions,owl-sirq"
>> +- reg: physical base address of the controller and length of memory mapped.
>> +- interrupt-controller: identifies the node as an interrupt controller
>> +- #interrupt-cells: specifies the number of cells needed to encode an interrupt
>> +  source, should be 2.
> 
>> +- actions,sirq-shared-reg: Applicable for S500 and S700 where SIRQ register
>> +  details are maintained at same offset/register.
>> +- actions,sirq-offset: register offset for SIRQ interrupts. When registers are
>> +  shared, all the three offsets will be same (S500 and S700).
> 
> You should have more specific compatible strings if there are
> differences and these settings can be implied by them.

This to meant to get the register offset because s500, s700 uses the
same external interrupt controller register to provide three or more
interrupt line. But this is not the case for s900.

So should it be "actions,sirq-offset-reg"?

> 
>> +- actions,sirq-clk-sel: external interrupt controller can be either
>> +  connected to 32Khz or 24Mhz external/internal clock. This needs
>> +  to be configured for per SIRQ line. Failing defaults to 32Khz clock.
> 
> What are the valid values?
> 
>> +
>> +Example for S900:
>> +
>> +sirq: interrupt-controller@e01b0000 {
>> +	compatible = "actions,owl-sirq";
>> +	reg = <0 0xe01b0000 0 0x1000>;
>> +	interrupt-controller;
>> +	#interrupt-cells = <2>;
>> +	actions,sirq-clk-sel = <0 0 0>;
> 
> If 0 is 32khz, then having this is pointless. But I can't tell what the
> values correspond to.

Thanks, clock selection will be removed and defaults to 24MHz.

> 
>> +	actions,sirq-offset = <0x200 0x528 0x52c>;
>> +};
>> +
>> +Example for S500 and S700:
>> +
>> +sirq: interrupt-controller@e01b0000 {
>> +	compatible = "actions,owl-sirq";
>> +	reg = <0 0xe01b0000 0 0x1000>;
>> +	interrupt-controller;
>> +	#interrupt-cells = <2>;
>> +	actions,sirq-shared-reg;
>> +	actions,sirq-clk-sel = <0 0 0>;
>> +	actions,sirq-offset = <0x200 0x200 0x200>;
>> +};
>> -- 
>> 2.14.4
>>
> 

-- 
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@denx.de

Re: [PATCH v2 2/3] drivers/irqchip: Add Actions external interrupts support

[Marc Zyngier]

On Mon, 12 Nov 2018 10:32:51 +0000,
Parthiban Nallathambi <pn@denx.de> wrote:
> 
> 
> 
> On 11/8/18 6:03 PM, Marc Zyngier wrote:
> > On 26/08/18 16:20, 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@denx.de>
> >>>> Signed-off-by: Saravanan Sekar <sravanhome@gmail.com>
> >>>> ---
> >>>>   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@actions-semi.com>
> >>>> + *
> >>>> + * Author: Parthiban Nallathambi <pn@denx.de>
> >>>> + * Author: Saravanan Sekar <sravanhome@gmail.com>
> >>>> + */
> >>>> +
> >>>> +#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.
> > 
> > This is normally shown as a property from DT, and is relative to the
> > parent irqchip. And I don't understand what you mean by "offset into the
> > register". The only use of this is to allocate the corresponding GIC
> 
> We have two SoC's (s500, s700) with shared external interrupt control
> register and one (s900) with dedicated register for each external
> interrupt line. So the DT property "actions,sirq-offset" was introduced
> to access the register.
> 
> In case of s500, s700 when it's shared, the idea is to use the "hwirq"
> variable value to offset into the control register INTC_EXTCTL. Even if
> 3 cell GIC value is directly used like
> 
> interrupts = <GIC_SPI 2 IRQ_TYPE_LEVEL_HIGH>;
> 
> then hwirq - 13 is needed internally everywhere in this driver.
> 
> In short, this value is defined inside driver for the ease of referring
> the offset with the register.
> 
> Yes, it is possible to change the driver logic and use 3 cell interrupts
> from DT.
> 
> > interrupt, and this definitely shouldn't be harcoded.
> > 
> >> 
> >> 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.
> > 
> > If you don't have any way to test it, is it worth it to have that code
> > in? I'd prefer you add code that actually works, even if that's for a
> > subset of the capability of the HW, rather than add code that cannot be
> > exercised.
> 
> Ok, it wasn't case now. I have the hardware connected on these lines and
> tested ok with the implementation moved to .irq.eoi.
> 
> > 
> >> 
> >>> 
> >>>> +}
> >>>> +
> >>>> +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?
> > 
> > I think this should be entirely hidden from the interrupt controller,
> > and set by firmware or by the platform clock setup.
> 
> Agreed!
> 
> > 
> >> 
> >>> 
> >>>> +
> >>>> +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!
> > 
> > I must say I've lost track of this driver a while ago. Can you please
> > send whatever you have come up with, and we'll take it from there.
> 
> Sure, there is one thing changed, tested the eoi part with the PMIC
> connected on this interrupt line and it works.
> 
> Should I send v3 using 3 interrupt cells and re-define the usage of
> hwirq to offsetting?

I don't really care how this is expressed in the device-tree (Rob can
certainly help you define something that works), but I don't want to
see hardcoded values in the driver.

Thanks,

	M.

-- 
Jazz is not dead, it just smell funny.