Messages in this thread | | | Subject | Re: [PATCH v2 2/3] drivers/irqchip: Add Actions external interrupts support | From | Parthiban Nallathambi <> | Date | Thu, 20 Sep 2018 11:42:17 +0200 |
| |
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
| |