On Mon, 12 Nov 2018 10:32:51 +0000,
Parthiban Nallathambi <pn@xxxxxxx> 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@xxxxxxx>
> >>>> Signed-off-by: Saravanan Sekar <sravanhome@xxxxxxxxx>
> >>>> ---
> >>>> 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@xxxxxxxxxxxxxxxx>
> >>>> + *
> >>>> + * Author: Parthiban Nallathambi <pn@xxxxxxx>
> >>>> + * Author: Saravanan Sekar <sravanhome@xxxxxxxxx>
> >>>> + */
> >>>> +
> >>>> +#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.
