lkml.org 
[lkml]   [2018]   [Sep]   [20]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH v2 2/3] drivers/irqchip: Add Actions external interrupts support
From
Date
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

\
 
 \ /
  Last update: 2018-09-20 11:45    [W:0.115 / U:0.092 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site