Date:   Thu, 20 Sep 2018 11:42:17 +0200
From:   Parthiban Nallathambi <pn@...x.de>
To:     Marc Zyngier <marc.zyngier@....com>, tglx@...utronix.de,
        jason@...edaemon.net, robh+dt@...nel.org, mark.rutland@....com,
        afaerber@...e.de, catalin.marinas@....com, will.deacon@....com,
        manivannan.sadhasivam@...aro.org
Cc:     pn@...x.de, linux-kernel@...r.kernel.org,
        devicetree@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
        sravanhome@...il.com, thomas.liau@...ions-semi.com,
        mp-cs@...ions-semi.com, linux@...ietech.com,
        edgar.righi@...tec.org.br, laisa.costa@...tec.org.br,
        guilherme.simoes@...tec.org.br, mkzuffo@....usp.br
Subject: Re: [PATCH v2 2/3] drivers/irqchip: Add Actions external interrupts
 support

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@...x.de>
>>> Signed-off-by: Saravanan Sekar <sravanhome@...il.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@...ions-semi.com>
>>> + *
>>> + * Author: Parthiban Nallathambi <pn@...x.de>
>>> + * Author: Saravanan Sekar <sravanhome@...il.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

Powered by blists - more mailing lists