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

[Marc Zyngier <marc.zyngier@arm.com>]

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.