Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Subject: Re: [PATCH v2 2/3] drivers/irqchip: Add Actions external interrupts
 support
To:     Parthiban Nallathambi <pn@denx.de>, tglx@linutronix.de,
        jason@lakedaemon.net, robh+dt@kernel.org, mark.rutland@arm.com,
        afaerber@suse.de, catalin.marinas@arm.com, will.deacon@arm.com,
        manivannan.sadhasivam@linaro.org
Cc:     linux-kernel@vger.kernel.org, devicetree@vger.kernel.org,
        linux-arm-kernel@lists.infradead.org, sravanhome@gmail.com,
        thomas.liau@actions-semi.com, mp-cs@actions-semi.com,
        linux@cubietech.com, edgar.righi@lsitec.org.br,
        laisa.costa@lsitec.org.br, guilherme.simoes@lsitec.org.br,
        mkzuffo@lsi.usp.br
References: <20180812122215.1079590-1-pn@denx.de>
 <20180812122215.1079590-3-pn@denx.de>
 <64b9ae63-c2b9-a2e0-f648-4bf80f34a57a@arm.com>
 <e8ff35c7-813c-2597-ab51-781336d551c5@denx.de>
From:   Marc Zyngier <marc.zyngier@arm.com>
Openpgp: preference=signencrypt
Autocrypt: addr=marc.zyngier@arm.com; prefer-encrypt=mutual; keydata=
 xsFNBE6Jf0UBEADLCxpix34Ch3kQKA9SNlVQroj9aHAEzzl0+V8jrvT9a9GkK+FjBOIQz4KE
 g+3p+lqgJH4NfwPm9H5I5e3wa+Scz9wAqWLTT772Rqb6hf6kx0kKd0P2jGv79qXSmwru28vJ
 t9NNsmIhEYwS5eTfCbsZZDCnR31J6qxozsDHpCGLHlYym/VbC199Uq/pN5gH+5JHZyhyZiNW
 ozUCjMqC4eNW42nYVKZQfbj/k4W9xFfudFaFEhAf/Vb1r6F05eBP1uopuzNkAN7vqS8XcgQH
 qXI357YC4ToCbmqLue4HK9+2mtf7MTdHZYGZ939OfTlOGuxFW+bhtPQzsHiW7eNe0ew0+LaL
 3wdNzT5abPBscqXWVGsZWCAzBmrZato+Pd2bSCDPLInZV0j+rjt7MWiSxEAEowue3IcZA++7
 ifTDIscQdpeKT8hcL+9eHLgoSDH62SlubO/y8bB1hV8JjLW/jQpLnae0oz25h39ij4ijcp8N
 t5slf5DNRi1NLz5+iaaLg4gaM3ywVK2VEKdBTg+JTg3dfrb3DH7ctTQquyKun9IVY8AsxMc6
 lxl4HxrpLX7HgF10685GG5fFla7R1RUnW5svgQhz6YVU33yJjk5lIIrrxKI/wLlhn066mtu1
 DoD9TEAjwOmpa6ofV6rHeBPehUwMZEsLqlKfLsl0PpsJwov8TQARAQABzSNNYXJjIFp5bmdp
 ZXIgPG1hcmMuenluZ2llckBhcm0uY29tPsLBewQTAQIAJQIbAwYLCQgHAwIGFQgCCQoLBBYC
 AwECHgECF4AFAk6NvYYCGQEACgkQI9DQutE9ekObww/+NcUATWXOcnoPflpYG43GZ0XjQLng
 LQFjBZL+CJV5+1XMDfz4ATH37cR+8gMO1UwmWPv5tOMKLHhw6uLxGG4upPAm0qxjRA/SE3LC
 22kBjWiSMrkQgv5FDcwdhAcj8A+gKgcXBeyXsGBXLjo5UQOGvPTQXcqNXB9A3ZZN9vS6QUYN
 TXFjnUnzCJd+PVI/4jORz9EUVw1q/+kZgmA8/GhfPH3xNetTGLyJCJcQ86acom2liLZZX4+1
 6Hda2x3hxpoQo7pTu+XA2YC4XyUstNDYIsE4F4NVHGi88a3N8yWE+Z7cBI2HjGvpfNxZnmKX
 6bws6RQ4LHDPhy0yzWFowJXGTqM/e79c1UeqOVxKGFF3VhJJu1nMlh+5hnW4glXOoy/WmDEM
 UMbl9KbJUfo+GgIQGMp8mwgW0vK4HrSmevlDeMcrLdfbbFbcZLNeFFBn6KqxFZaTd+LpylIH
 bOPN6fy1Dxf7UZscogYw5Pt0JscgpciuO3DAZo3eXz6ffj2NrWchnbj+SpPBiH4srfFmHY+Y
 LBemIIOmSqIsjoSRjNEZeEObkshDVG5NncJzbAQY+V3Q3yo9og/8ZiaulVWDbcpKyUpzt7pv
 cdnY3baDE8ate/cymFP5jGJK++QCeA6u6JzBp7HnKbngqWa6g8qDSjPXBPCLmmRWbc5j0lvA
 6ilrF8nOwU0ETol/RQEQAM/2pdLYCWmf3rtIiP8Wj5NwyjSL6/UrChXtoX9wlY8a4h3EX6E3
 64snIJVMLbyr4bwdmPKULlny7T/R8dx/mCOWu/DztrVNQiXWOTKJnd/2iQblBT+W5W8ep/nS
 w3qUIckKwKdplQtzSKeE+PJ+GMS+DoNDDkcrVjUnsoCEr0aK3cO6g5hLGu8IBbC1CJYSpple
 VVb/sADnWF3SfUvJ/l4K8Uk4B4+X90KpA7U9MhvDTCy5mJGaTsFqDLpnqp/yqaT2P7kyMG2E
 w+eqtVIqwwweZA0S+tuqput5xdNAcsj2PugVx9tlw/LJo39nh8NrMxAhv5aQ+JJ2I8UTiHLX
 QvoC0Yc/jZX/JRB5r4x4IhK34Mv5TiH/gFfZbwxd287Y1jOaD9lhnke1SX5MXF7eCT3cgyB+
 hgSu42w+2xYl3+rzIhQqxXhaP232t/b3ilJO00ZZ19d4KICGcakeiL6ZBtD8TrtkRiewI3v0
 o8rUBWtjcDRgg3tWx/PcJvZnw1twbmRdaNvsvnlapD2Y9Js3woRLIjSAGOijwzFXSJyC2HU1
 AAuR9uo4/QkeIrQVHIxP7TJZdJ9sGEWdeGPzzPlKLHwIX2HzfbdtPejPSXm5LJ026qdtJHgz
 BAb3NygZG6BH6EC1NPDQ6O53EXorXS1tsSAgp5ZDSFEBklpRVT3E0NrDABEBAAHCwV8EGAEC
 AAkFAk6Jf0UCGwwACgkQI9DQutE9ekMLBQ//U+Mt9DtFpzMCIHFPE9nNlsCm75j22lNiw6mX
 mx3cUA3pl+uRGQr/zQC5inQNtjFUmwGkHqrAw+SmG5gsgnM4pSdYvraWaCWOZCQCx1lpaCOl
 MotrNcwMJTJLQGc4BjJyOeSH59HQDitKfKMu/yjRhzT8CXhys6R0kYMrEN0tbe1cFOJkxSbV
 0GgRTDF4PKyLT+RncoKxQe8lGxuk5614aRpBQa0LPafkirwqkUtxsPnarkPUEfkBlnIhAR8L
 kmneYLu0AvbWjfJCUH7qfpyS/FRrQCoBq9QIEcf2v1f0AIpA27f9KCEv5MZSHXGCdNcbjKw1
 39YxYZhmXaHFKDSZIC29YhQJeXWlfDEDq6nIhvurZy3mSh2OMQgaIoFexPCsBBOclH8QUtMk
 a3jW/qYyrV+qUq9Wf3SKPrXf7B3xB332jFCETbyZQXqmowV+2b3rJFRWn5hK5B+xwvuxKyGq
 qDOGjof2dKl2zBIxbFgOclV7wqCVkhxSJi/QaOj2zBqSNPXga5DWtX3ekRnJLa1+ijXxmdjz
 hApihi08gwvP5G9fNGKQyRETePEtEAWt0b7dOqMzYBYGRVr7uS4uT6WP7fzOwAJC4lU7ZYWZ
 yVshCa0IvTtp1085RtT3qhh9mobkcZ+7cQOY+Tx2RGXS9WeOh2jZjdoWUv6CevXNQyOUXMM=
Organization: ARM Ltd
Message-ID: <769542dc-0fc4-cb24-6640-aad6672af0fb@arm.com>
Date:   Thu, 8 Nov 2018 17:03:26 +0000
User-Agent: Mozilla/5.0 (X11; Linux aarch64; rv:52.0) Gecko/20100101
 Thunderbird/52.9.1
MIME-Version: 1.0
In-Reply-To: <e8ff35c7-813c-2597-ab51-781336d551c5@denx.de>
Content-Type: text/plain; charset=utf-8
Content-Language: en-GB
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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