diff for duplicates of <43f5cba1199f89cde68c8a577103f28b@kernel.org> diff --git a/a/1.txt b/N1/1.txt index 9a3c27b..c18abca 100644 --- a/a/1.txt +++ b/N1/1.txt @@ -1,284 +1,335 @@ -On 2020-08-03 07:22, Mark-PK Tsai wrote: -> Add mt58xx interrupt controller support using hierarchy irq -> domain. -> -> Signed-off-by: Mark-PK Tsai <mark-pk.tsai@mediatek.com> -> --- -> drivers/irqchip/Kconfig | 7 ++ -> drivers/irqchip/Makefile | 1 + -> drivers/irqchip/irq-mt58xx.c | 196 +++++++++++++++++++++++++++++++++++ -> 3 files changed, 204 insertions(+) -> create mode 100644 drivers/irqchip/irq-mt58xx.c -> -> diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig -> index 216b3b8392b5..00453af78be0 100644 -> --- a/drivers/irqchip/Kconfig -> +++ b/drivers/irqchip/Kconfig -> @@ -572,4 +572,11 @@ config LOONGSON_PCH_MSI -> help -> Support for the Loongson PCH MSI Controller. -> -> +config MT58XX_IRQ -> + bool "MT58XX IRQ" -> + select IRQ_DOMAIN -> + select IRQ_DOMAIN_HIERARCHY -> + help -> + Support Mediatek MT58XX Interrupt Controller. -> + -> endmenu -> diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile -> index 133f9c45744a..5062e9bfa92d 100644 -> --- a/drivers/irqchip/Makefile -> +++ b/drivers/irqchip/Makefile -> @@ -111,3 +111,4 @@ obj-$(CONFIG_LOONGSON_HTPIC) += -> irq-loongson-htpic.o -> obj-$(CONFIG_LOONGSON_HTVEC) += irq-loongson-htvec.o -> obj-$(CONFIG_LOONGSON_PCH_PIC) += irq-loongson-pch-pic.o -> obj-$(CONFIG_LOONGSON_PCH_MSI) += irq-loongson-pch-msi.o -> +obj-$(CONFIG_MT58XX_IRQ) += irq-mt58xx.o -> diff --git a/drivers/irqchip/irq-mt58xx.c -> b/drivers/irqchip/irq-mt58xx.c -> new file mode 100644 -> index 000000000000..e45ad023afa6 -> --- /dev/null -> +++ b/drivers/irqchip/irq-mt58xx.c -> @@ -0,0 +1,196 @@ -> +// SPDX-License-Identifier: (GPL-2.0 OR BSD-3-Clause) -> +/* -> + * Copyright (c) 2020 MediaTek Inc. -> + * Author Mark-PK Tsai <mark-pk.tsai@mediatek.com> -> + */ -> + -> +#include <linux/interrupt.h> -> +#include <linux/io.h> -> +#include <linux/irq.h> -> +#include <linux/irqchip.h> -> +#include <linux/irqdomain.h> -> +#include <linux/of.h> -> +#include <linux/of_address.h> -> +#include <linux/of_irq.h> -> +#include <linux/slab.h> -> + -> +#define INTC_MASK 0x0 -> +#define INTC_EOI 0x20 -> + -> +struct mtk_intc_chip_data { -> + char *name; -> + struct irq_chip chip; +From: Marc Zyngier <maz@kernel.org> -There is no need to embed a full struct irqchip per controller, see -below. - -> + unsigned int irq_start, nr_irqs; -> + void __iomem *base; -> +}; -> + -> +static void mtk_poke_irq(struct irq_data *d, u32 offset) -> +{ -> + struct mtk_intc_chip_data *cd = irq_data_get_irq_chip_data(d); -> + void __iomem *base = cd->base; -> + u8 index = (u8)irqd_to_hwirq(d); +> On 2020-08-03 07:22, Mark-PK Tsai wrote: +> > Add mt58xx interrupt controller support using hierarchy irq +> > domain. +> > +> > Signed-off-by: Mark-PK Tsai <mark-pk.tsai@mediatek.com> +> > --- +> > drivers/irqchip/Kconfig | 7 ++ +> > drivers/irqchip/Makefile | 1 + +> > drivers/irqchip/irq-mt58xx.c | 196 +++++++++++++++++++++++++++++++++++ +> > 3 files changed, 204 insertions(+) +> > create mode 100644 drivers/irqchip/irq-mt58xx.c +> > +> > diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig +> > index 216b3b8392b5..00453af78be0 100644 +> > --- a/drivers/irqchip/Kconfig +> > +++ b/drivers/irqchip/Kconfig +> > @@ -572,4 +572,11 @@ config LOONGSON_PCH_MSI +> > help +> > Support for the Loongson PCH MSI Controller. +> > +> > +config MT58XX_IRQ +> > + bool "MT58XX IRQ" +> > + select IRQ_DOMAIN +> > + select IRQ_DOMAIN_HIERARCHY +> > + help +> > + Support Mediatek MT58XX Interrupt Controller. +> > + +> > endmenu +> > diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile +> > index 133f9c45744a..5062e9bfa92d 100644 +> > --- a/drivers/irqchip/Makefile +> > +++ b/drivers/irqchip/Makefile +> > @@ -111,3 +111,4 @@ obj-$(CONFIG_LOONGSON_HTPIC) += +> > irq-loongson-htpic.o +> > obj-$(CONFIG_LOONGSON_HTVEC) += irq-loongson-htvec.o +> > obj-$(CONFIG_LOONGSON_PCH_PIC) += irq-loongson-pch-pic.o +> > obj-$(CONFIG_LOONGSON_PCH_MSI) += irq-loongson-pch-msi.o +> > +obj-$(CONFIG_MT58XX_IRQ) += irq-mt58xx.o +> > diff --git a/drivers/irqchip/irq-mt58xx.c +> > b/drivers/irqchip/irq-mt58xx.c +> > new file mode 100644 +> > index 000000000000..e45ad023afa6 +> > --- /dev/null +> > +++ b/drivers/irqchip/irq-mt58xx.c +> > @@ -0,0 +1,196 @@ +> > +// SPDX-License-Identifier: (GPL-2.0 OR BSD-3-Clause) +> > +/* +> > + * Copyright (c) 2020 MediaTek Inc. +> > + * Author Mark-PK Tsai <mark-pk.tsai@mediatek.com> +> > + */ +> > + +> > +#include <linux/interrupt.h> +> > +#include <linux/io.h> +> > +#include <linux/irq.h> +> > +#include <linux/irqchip.h> +> > +#include <linux/irqdomain.h> +> > +#include <linux/of.h> +> > +#include <linux/of_address.h> +> > +#include <linux/of_irq.h> +> > +#include <linux/slab.h> +> > + +> > +#define INTC_MASK 0x0 +> > +#define INTC_EOI 0x20 +> > + +> > +struct mtk_intc_chip_data { +> > + char *name; +> > + struct irq_chip chip; +> +> There is no need to embed a full struct irqchip per controller, see +> below. -Why the restrictive type? Why isn't unsigned int good enough? +We want to distinguish which controller the device interrupts are belong to +by "cat /proc/interrupts". +And if all the controller share the same struct, the name field will be the +same. +Do you have suggestion for this? -> + u16 val, mask; -> + -> + mask = 1 << (index % 16); -> + val = readw_relaxed(base + offset + (index / 16) * 4) | mask; -> + writew_relaxed(val, base + offset + (index / 16) * 4); +> +> > + unsigned int irq_start, nr_irqs; +> > + void __iomem *base; +> > + +> }; +> > + +> > +static void mtk_poke_irq(struct irq_data *d, u32 offset) +> > +{ +> > + struct mtk_intc_chip_data *cd = irq_data_get_irq_chip_data(d); +> > + void __iomem *base = cd->base; +> > + u8 index = (u8)irqd_to_hwirq(d); +> +> Why the restrictive type? Why isn't unsigned int good enough? -RMW without locking, you will end-up with corruption. -Please store the address calculation in a temporaty variable to make it -more readable +You're right, unsigned int is ok. +I'll fix it in patch v2. -> +} -> + -> +static void mtk_clear_irq(struct irq_data *d, u32 offset) -> +{ -> + struct mtk_intc_chip_data *cd = irq_data_get_irq_chip_data(d); -> + void __iomem *base = cd->base; -> + u8 index = (u8)irqd_to_hwirq(d); -> + u16 val, mask; -> + -> + mask = 1 << (index % 16); -> + val = readw_relaxed(base + offset + (index / 16) * 4) & ~mask; -> + writew_relaxed(val, base + offset + (index / 16) * 4); +> +> > + u16 val, mask; +> > + +> > + mask = 1 << (index % 16); +> > + val = readw_relaxed(base + offset + (index / 16) * 4) | mask; +> > + writew_relaxed(val, base + offset + (index / 16) * 4); +> +> RMW without locking, you will end-up with corruption. +> Please store the address calculation in a temporaty variable to make it +> more readable +> -Same comments. +Thanks for the comment, I will fix it in pacth v2. -> +} -> + -> +static void mtk_intc_mask_irq(struct irq_data *d) -> +{ -> + mtk_poke_irq(d, INTC_MASK); -> + irq_chip_mask_parent(d); -> +} -> + -> +static void mtk_intc_unmask_irq(struct irq_data *d) -> +{ -> + mtk_clear_irq(d, INTC_MASK); -> + irq_chip_unmask_parent(d); -> +} -> + -> +static void mtk_intc_eoi_irq(struct irq_data *d) -> +{ -> + mtk_poke_irq(d, INTC_EOI); -> + irq_chip_eoi_parent(d); -> +} -> + -> +static struct irq_chip mtk_intc_chip = { -> + .irq_mask = mtk_intc_mask_irq, -> + .irq_unmask = mtk_intc_unmask_irq, -> + .irq_eoi = mtk_intc_eoi_irq, -> + .irq_get_irqchip_state = irq_chip_get_parent_state, -> + .irq_set_irqchip_state = irq_chip_set_parent_state, -> + .irq_set_affinity = irq_chip_set_affinity_parent, -> + .irq_set_vcpu_affinity = irq_chip_set_vcpu_affinity_parent, +> > + +> } +> > + +> > +static void mtk_clear_irq(struct irq_data *d, u32 offset) +> > +{ +> > + struct mtk_intc_chip_data *cd = irq_data_get_irq_chip_data(d); +> > + void __iomem *base = cd->base; +> > + u8 index = (u8)irqd_to_hwirq(d); +> > + u16 val, mask; +> > + +> > + mask = 1 << (index % 16); +> > + val = readw_relaxed(base + offset + (index / 16) * 4) & ~mask; +> > + writew_relaxed(val, base + offset + (index / 16) * 4); +> +> Same comments. +> -How about retrigger? +You're right, unsigned int is ok. +I'll fix it in patch v2. -> + .irq_set_type = irq_chip_set_type_parent, -> + .flags = IRQCHIP_SET_TYPE_MASKED | -> + IRQCHIP_SKIP_SET_WAKE | -> + IRQCHIP_MASK_ON_SUSPEND, -> +}; -> + -> +static int mt58xx_intc_domain_translate(struct irq_domain *d, -> + struct irq_fwspec *fwspec, -> + unsigned long *hwirq, -> + unsigned int *type) -> +{ -> + if (is_of_node(fwspec->fwnode)) { -> + if (fwspec->param_count != 3) -> + return -EINVAL; -> + -> + /* No PPI should point to this domain */ -> + if (fwspec->param[0] != 0) -> + return -EINVAL; -> + -> + *hwirq = fwspec->param[1]; -> + *type = fwspec->param[2] & IRQ_TYPE_SENSE_MASK; -> + return 0; -> + } -> + -> + return -EINVAL; -> +} -> + -> +static int mt58xx_intc_domain_alloc(struct irq_domain *domain, -> unsigned int virq, -> + unsigned int nr_irqs, void *data) -> +{ -> + int i; -> + irq_hw_number_t hwirq; -> + struct irq_fwspec parent_fwspec, *fwspec = data; -> + struct mtk_intc_chip_data *cd = (struct mtk_intc_chip_data -> *)domain->host_data; -> + -> + /* Not GIC compliant */ -> + if (fwspec->param_count != 3) -> + return -EINVAL; -> + -> + /* No PPI should point to this domain */ -> + if (fwspec->param[0]) -> + return -EINVAL; -> + -> + if (fwspec->param[1] >= cd->nr_irqs) -> + return -EINVAL; -> + -> + hwirq = fwspec->param[1]; -> + for (i = 0; i < nr_irqs; i++) -> + irq_domain_set_hwirq_and_chip(domain, virq + i, hwirq + i, -> + &cd->chip, -> + domain->host_data); -> + -> + parent_fwspec = *fwspec; -> + parent_fwspec.fwnode = domain->parent->fwnode; -> + parent_fwspec.param[1] = cd->irq_start + hwirq; -> + return irq_domain_alloc_irqs_parent(domain, virq, nr_irqs, -> &parent_fwspec); -> +} -> + -> +static const struct irq_domain_ops mt58xx_intc_domain_ops = { -> + .translate = mt58xx_intc_domain_translate, -> + .alloc = mt58xx_intc_domain_alloc, -> + .free = irq_domain_free_irqs_common, -> +}; -> + -> +int __init -> +mt58xx_intc_of_init(struct device_node *dn, struct device_node -> *parent) -> +{ -> + static int nr_intc; -> + struct irq_domain *domain, *domain_parent; -> + struct mtk_intc_chip_data *cd; -> + unsigned int irq_start, irq_end; -> + -> + domain_parent = irq_find_host(parent); -> + if (!domain_parent) { -> + pr_err("mt58xx-intc: interrupt-parent not found\n"); -> + return -EINVAL; -> + } -> + -> + cd = kzalloc(sizeof(*cd), GFP_KERNEL); -> + if (!cd) -> + return -ENOMEM; -> + -> + cd->chip = mtk_intc_chip; -> + if (of_property_read_u32_index(dn, "mediatek,irqs-map-range", 0, -> &irq_start) || -> + of_property_read_u32_index(dn, "mediatek,irqs-map-range", 1, -> &irq_end)) { -> + kfree(cd); -> + return -EINVAL; -> + } -> + -> + if (of_property_read_bool(dn, "mediatek,intc-no-eoi")) -> + cd->chip.irq_eoi = irq_chip_eoi_parent; +> > + +> } +> > + +> > +static void mtk_intc_mask_irq(struct irq_data *d) +> > +{ +> > + mtk_poke_irq(d, INTC_MASK); +> > + irq_chip_mask_parent(d); +> > + +> } +> > + +> > +static void mtk_intc_unmask_irq(struct irq_data *d) +> > +{ +> > + mtk_clear_irq(d, INTC_MASK); +> > + irq_chip_unmask_parent(d); +> > + +> } +> > + +> > +static void mtk_intc_eoi_irq(struct irq_data *d) +> > +{ +> > + mtk_poke_irq(d, INTC_EOI); +> > + irq_chip_eoi_parent(d); +> > + +> } +> > + +> > +static struct irq_chip mtk_intc_chip = { +> > + .irq_mask = mtk_intc_mask_irq, +> > + .irq_unmask = mtk_intc_unmask_irq, +> > + .irq_eoi = mtk_intc_eoi_irq, +> > + .irq_get_irqchip_state = irq_chip_get_parent_state, +> > + .irq_set_irqchip_state = irq_chip_set_parent_state, +> > + .irq_set_affinity = irq_chip_set_affinity_parent, +> > + .irq_set_vcpu_affinity = irq_chip_set_vcpu_affinity_parent, +> +> How about retrigger? +> -No. Just add a flag to your chip data structure, and check for this -flag in your irq_eoi callback. Or provide two distinct irq_chip -structures that will only differ by the irq_eoi method. +What is retrigger means? +To be honest, I just try to direct all the irqchip ops implemented in +/drivers/irqchip/irq-gic.c to gic driver. +But "irq_set_vcpu_affinity" is not used in our projects now. +Should I remove ".irq_set_vcpu_affinity" here? -> + -> + cd->irq_start = irq_start; -> + cd->nr_irqs = irq_end - irq_start + 1; -> + cd->chip.name = kasprintf(GFP_KERNEL, "mt58xx-intc-%d", nr_intc++); +> > + .irq_set_type = irq_chip_set_type_parent, +> > + .flags = IRQCHIP_SET_TYPE_MASKED | +> > + IRQCHIP_SKIP_SET_WAKE | +> > + IRQCHIP_MASK_ON_SUSPEND, +> > + +> }; +> > + +> > +static int mt58xx_intc_domain_translate(struct irq_domain *d, +> > + struct irq_fwspec *fwspec, +> > + unsigned long *hwirq, +> > + unsigned int *type) +> > +{ +> > + if (is_of_node(fwspec->fwnode)) { +> > + if (fwspec->param_count != 3) +> > + return -EINVAL; +> > + +> > + /* No PPI should point to this domain */ +> > + if (fwspec->param[0] != 0) +> > + return -EINVAL; +> > + +> > + *hwirq = fwspec->param[1]; +> > + *type = fwspec->param[2] & IRQ_TYPE_SENSE_MASK; +> > + return 0; +> > + +> } +> > + +> > + return -EINVAL; +> > + +> } +> > + +> > +static int mt58xx_intc_domain_alloc(struct irq_domain *domain, +> > unsigned int virq, +> > + unsigned int nr_irqs, void *data) +> > +{ +> > + int i; +> > + irq_hw_number_t hwirq; +> > + struct irq_fwspec parent_fwspec, *fwspec = data; +> > + struct mtk_intc_chip_data *cd = (struct mtk_intc_chip_data +> > *)domain->host_data; +> > + +> > + /* Not GIC compliant */ +> > + if (fwspec->param_count != 3) +> > + return -EINVAL; +> > + +> > + /* No PPI should point to this domain */ +> > + if (fwspec->param[0]) +> > + return -EINVAL; +> > + +> > + if (fwspec->param[1] >= cd->nr_irqs) +> > + return -EINVAL; +> > + +> > + hwirq = fwspec->param[1]; +> > + for (i = 0; i < nr_irqs; i++) +> > + irq_domain_set_hwirq_and_chip(domain, virq + i, hwirq + i, +> > + &cd->chip, +> > + domain->host_data); +> > + +> > + parent_fwspec = *fwspec; +> > + parent_fwspec.fwnode = domain->parent->fwnode; +> > + parent_fwspec.param[1] = cd->irq_start + hwirq; +> > + return irq_domain_alloc_irqs_parent(domain, virq, nr_irqs, +> > &parent_fwspec); +> > + +> } +> > + +> > +static const struct irq_domain_ops mt58xx_intc_domain_ops = { +> > + .translate = mt58xx_intc_domain_translate, +> > + .alloc = mt58xx_intc_domain_alloc, +> > + .free = irq_domain_free_irqs_common, +> > + +> }; +> > + +> > +int __init +> > +mt58xx_intc_of_init(struct device_node *dn, struct device_node +> > *parent) +> > +{ +> > + static int nr_intc; +> > + struct irq_domain *domain, *domain_parent; +> > + struct mtk_intc_chip_data *cd; +> > + unsigned int irq_start, irq_end; +> > + +> > + domain_parent = irq_find_host(parent); +> > + if (!domain_parent) { +> > + pr_err("mt58xx-intc: interrupt-parent not found\n"); +> > + return -EINVAL; +> > + +> } +> > + +> > + cd = kzalloc(sizeof(*cd), GFP_KERNEL); +> > + if (!cd) +> > + return -ENOMEM; +> > + +> > + cd->chip = mtk_intc_chip; +> > + if (of_property_read_u32_index(dn, "mediatek,irqs-map-range", 0, +> > &irq_start) || +> > + of_property_read_u32_index(dn, "mediatek,irqs-map-range", 1, +> > &irq_end)) { +> > + kfree(cd); +> > + return -EINVAL; +> > + +> } +> > + +> > + if (of_property_read_bool(dn, "mediatek,intc-no-eoi")) +> > + cd->chip.irq_eoi = irq_chip_eoi_parent; +> +> No. Just add a flag to your chip data structure, and check for this +> flag in your irq_eoi callback. Or provide two distinct irq_chip +> structures that will only differ by the irq_eoi method. -Neither. That's not useful, and is a waste of memory. Stick to constant -names in the irq_chip structure. +Thanks for the comment, I will modify it in patch v2. -> + if (!cd->chip.name) { -> + kfree(cd); -> + return -ENOMEM; -> + } -> + -> + cd->base = of_iomap(dn, 0); -> + if (!cd->base) { -> + kfree(cd->chip.name); -> + kfree(cd); -> + return -ENOMEM; -> + } -> + -> + domain = irq_domain_add_hierarchy(domain_parent, 0, cd->nr_irqs, -> + dn, &mt58xx_intc_domain_ops, cd); -> + if (!domain) { -> + kfree(cd->chip.name); -> + iounmap(cd->base); -> + kfree(cd); -> + return -ENOMEM; -> + } -> + -> + return 0; -> +} -> + -> +IRQCHIP_DECLARE(mt58xx_intc, "mediatek,mt58xx-intc", -> mt58xx_intc_of_init); +> +> > + +> > + cd->irq_start = irq_start; +> > + cd->nr_irqs = irq_end - irq_start + 1; +> > + cd->chip.name = kasprintf(GFP_KERNEL, "mt58xx-intc-%d", nr_intc++); +> +> Neither. That's not useful, and is a waste of memory. Stick to constant +> names in the irq_chip structure. +> -On a side note, the merge window has just opened. Please refrain from -reposting this until -rc1. +Actually we have multiple irq controller in our SoCs. +And if we use the constant names in irq_chip structure, the information in +"/proc/interrupts" will be hard to understand because all the irqchip name +is the same. +Do you have any suggestion for this? -Thanks, +> > + if (!cd->chip.name) { +> > + kfree(cd); +> > + return -ENOMEM; +> > + +> } +> > + +> > + cd->base = of_iomap(dn, 0); +> > + if (!cd->base) { +> > + kfree(cd->chip.name); +> > + kfree(cd); +> > + return -ENOMEM; +> > + +> } +> > + +> > + domain = irq_domain_add_hierarchy(domain_parent, 0, cd->nr_irqs, +> > + dn, &mt58xx_intc_domain_ops, cd); +> > + if (!domain) { +> > + kfree(cd->chip.name); +> > + iounmap(cd->base); +> > + kfree(cd); +> > + return -ENOMEM; +> > + +> } +> > + +> > + return 0; +> > + +> } +> > + +> > +IRQCHIP_DECLARE(mt58xx_intc, "mediatek,mt58xx-intc", +> > mt58xx_intc_of_init); +> +> On a side note, the merge window has just opened. Please refrain from +> reposting this until -rc1. - M. --- -Jazz is not dead. It just smells funny... +Got it, and thanks for your comments. +I'll update the patch and post it after -rc1. diff --git a/a/content_digest b/N1/content_digest index 91c8225..0b97145 100644 --- a/a/content_digest +++ b/N1/content_digest @@ -1,304 +1,355 @@ - "ref\020200803062214.24076-1-mark-pk.tsai@mediatek.com\0" "ref\020200803062214.24076-2-mark-pk.tsai@mediatek.com\0" - "From\0Marc Zyngier <maz@kernel.org>\0" + "From\0Mark-PK Tsai <mark-pk.tsai@mediatek.com>\0" "Subject\0Re: [PATCH 1/2] irqchip: irq-mt58xx: Add mt58xx interrupt controller support\0" - "Date\0Mon, 03 Aug 2020 09:04:39 +0100\0" - "To\0Mark-PK Tsai <mark-pk.tsai@mediatek.com>\0" - "Cc\0yj.chiang@mediatek.com" - alix.wu@mediatek.com - tglx@linutronix.de - jason@lakedaemon.net - robh+dt@kernel.org - matthias.bgg@gmail.com - linux-kernel@vger.kernel.org - devicetree@vger.kernel.org - linux-arm-kernel@lists.infradead.org - " linux-mediatek@lists.infradead.org\0" + "Date\0Mon, 3 Aug 2020 23:03:37 +0800\0" + "To\0<maz@kernel.org>" + " Mark-PK Tsai <mark-pk.tsai@mediatek.com>\0" + "Cc\0<alix.wu@mediatek.com>" + <devicetree@vger.kernel.org> + <jason@lakedaemon.net> + <linux-arm-kernel@lists.infradead.org> + <linux-kernel@vger.kernel.org> + <linux-mediatek@lists.infradead.org> + <matthias.bgg@gmail.com> + <robh+dt@kernel.org> + <tglx@linutronix.de> + " <yj.chiang@mediatek.com>\0" "\00:1\0" "b\0" - "On 2020-08-03 07:22, Mark-PK Tsai wrote:\n" - "> Add mt58xx interrupt controller support using hierarchy irq\n" - "> domain.\n" - "> \n" - "> Signed-off-by: Mark-PK Tsai <mark-pk.tsai@mediatek.com>\n" - "> ---\n" - "> drivers/irqchip/Kconfig | 7 ++\n" - "> drivers/irqchip/Makefile | 1 +\n" - "> drivers/irqchip/irq-mt58xx.c | 196 +++++++++++++++++++++++++++++++++++\n" - "> 3 files changed, 204 insertions(+)\n" - "> create mode 100644 drivers/irqchip/irq-mt58xx.c\n" - "> \n" - "> diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig\n" - "> index 216b3b8392b5..00453af78be0 100644\n" - "> --- a/drivers/irqchip/Kconfig\n" - "> +++ b/drivers/irqchip/Kconfig\n" - "> @@ -572,4 +572,11 @@ config LOONGSON_PCH_MSI\n" - "> \thelp\n" - "> \t Support for the Loongson PCH MSI Controller.\n" - "> \n" - "> +config MT58XX_IRQ\n" - "> +\tbool \"MT58XX IRQ\"\n" - "> +\tselect IRQ_DOMAIN\n" - "> +\tselect IRQ_DOMAIN_HIERARCHY\n" - "> +\thelp\n" - "> +\t Support Mediatek MT58XX Interrupt Controller.\n" - "> +\n" - "> endmenu\n" - "> diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile\n" - "> index 133f9c45744a..5062e9bfa92d 100644\n" - "> --- a/drivers/irqchip/Makefile\n" - "> +++ b/drivers/irqchip/Makefile\n" - "> @@ -111,3 +111,4 @@ obj-$(CONFIG_LOONGSON_HTPIC)\t\t+= \n" - "> irq-loongson-htpic.o\n" - "> obj-$(CONFIG_LOONGSON_HTVEC)\t\t+= irq-loongson-htvec.o\n" - "> obj-$(CONFIG_LOONGSON_PCH_PIC)\t\t+= irq-loongson-pch-pic.o\n" - "> obj-$(CONFIG_LOONGSON_PCH_MSI)\t\t+= irq-loongson-pch-msi.o\n" - "> +obj-$(CONFIG_MT58XX_IRQ)\t\t+= irq-mt58xx.o\n" - "> diff --git a/drivers/irqchip/irq-mt58xx.c \n" - "> b/drivers/irqchip/irq-mt58xx.c\n" - "> new file mode 100644\n" - "> index 000000000000..e45ad023afa6\n" - "> --- /dev/null\n" - "> +++ b/drivers/irqchip/irq-mt58xx.c\n" - "> @@ -0,0 +1,196 @@\n" - "> +// SPDX-License-Identifier: (GPL-2.0 OR BSD-3-Clause)\n" - "> +/*\n" - "> + * Copyright (c) 2020 MediaTek Inc.\n" - "> + * Author Mark-PK Tsai <mark-pk.tsai@mediatek.com>\n" - "> + */\n" - "> +\n" - "> +#include <linux/interrupt.h>\n" - "> +#include <linux/io.h>\n" - "> +#include <linux/irq.h>\n" - "> +#include <linux/irqchip.h>\n" - "> +#include <linux/irqdomain.h>\n" - "> +#include <linux/of.h>\n" - "> +#include <linux/of_address.h>\n" - "> +#include <linux/of_irq.h>\n" - "> +#include <linux/slab.h>\n" - "> +\n" - "> +#define INTC_MASK\t0x0\n" - "> +#define INTC_EOI\t0x20\n" - "> +\n" - "> +struct mtk_intc_chip_data {\n" - "> +\tchar *name;\n" - "> +\tstruct irq_chip chip;\n" + "From: Marc Zyngier <maz@kernel.org>\n" "\n" - "There is no need to embed a full struct irqchip per controller, see \n" - "below.\n" - "\n" - "> +\tunsigned int irq_start, nr_irqs;\n" - "> +\tvoid __iomem *base;\n" - "> +};\n" - "> +\n" - "> +static void mtk_poke_irq(struct irq_data *d, u32 offset)\n" - "> +{\n" - "> +\tstruct mtk_intc_chip_data *cd = irq_data_get_irq_chip_data(d);\n" - "> +\tvoid __iomem *base = cd->base;\n" - "> +\tu8 index = (u8)irqd_to_hwirq(d);\n" + "> On 2020-08-03 07:22, Mark-PK Tsai wrote:\n" + "> > Add mt58xx interrupt controller support using hierarchy irq\n" + "> > domain.\n" + "> > \n" + "> > Signed-off-by: Mark-PK Tsai <mark-pk.tsai@mediatek.com>\n" + "> > ---\n" + "> > drivers/irqchip/Kconfig | 7 ++\n" + "> > drivers/irqchip/Makefile | 1 +\n" + "> > drivers/irqchip/irq-mt58xx.c | 196 +++++++++++++++++++++++++++++++++++\n" + "> > 3 files changed, 204 insertions(+)\n" + "> > create mode 100644 drivers/irqchip/irq-mt58xx.c\n" + "> > \n" + "> > diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig\n" + "> > index 216b3b8392b5..00453af78be0 100644\n" + "> > --- a/drivers/irqchip/Kconfig\n" + "> > +++ b/drivers/irqchip/Kconfig\n" + "> > @@ -572,4 +572,11 @@ config LOONGSON_PCH_MSI\n" + "> > \thelp\n" + "> > \t Support for the Loongson PCH MSI Controller.\n" + "> > \n" + "> > +config MT58XX_IRQ\n" + "> > +\tbool \"MT58XX IRQ\"\n" + "> > +\tselect IRQ_DOMAIN\n" + "> > +\tselect IRQ_DOMAIN_HIERARCHY\n" + "> > +\thelp\n" + "> > +\t Support Mediatek MT58XX Interrupt Controller.\n" + "> > +\n" + "> > endmenu\n" + "> > diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile\n" + "> > index 133f9c45744a..5062e9bfa92d 100644\n" + "> > --- a/drivers/irqchip/Makefile\n" + "> > +++ b/drivers/irqchip/Makefile\n" + "> > @@ -111,3 +111,4 @@ obj-$(CONFIG_LOONGSON_HTPIC)\t\t+= \n" + "> > irq-loongson-htpic.o\n" + "> > obj-$(CONFIG_LOONGSON_HTVEC)\t\t+= irq-loongson-htvec.o\n" + "> > obj-$(CONFIG_LOONGSON_PCH_PIC)\t\t+= irq-loongson-pch-pic.o\n" + "> > obj-$(CONFIG_LOONGSON_PCH_MSI)\t\t+= irq-loongson-pch-msi.o\n" + "> > +obj-$(CONFIG_MT58XX_IRQ)\t\t+= irq-mt58xx.o\n" + "> > diff --git a/drivers/irqchip/irq-mt58xx.c \n" + "> > b/drivers/irqchip/irq-mt58xx.c\n" + "> > new file mode 100644\n" + "> > index 000000000000..e45ad023afa6\n" + "> > --- /dev/null\n" + "> > +++ b/drivers/irqchip/irq-mt58xx.c\n" + "> > @@ -0,0 +1,196 @@\n" + "> > +// SPDX-License-Identifier: (GPL-2.0 OR BSD-3-Clause)\n" + "> > +/*\n" + "> > + * Copyright (c) 2020 MediaTek Inc.\n" + "> > + * Author Mark-PK Tsai <mark-pk.tsai@mediatek.com>\n" + "> > + */\n" + "> > +\n" + "> > +#include <linux/interrupt.h>\n" + "> > +#include <linux/io.h>\n" + "> > +#include <linux/irq.h>\n" + "> > +#include <linux/irqchip.h>\n" + "> > +#include <linux/irqdomain.h>\n" + "> > +#include <linux/of.h>\n" + "> > +#include <linux/of_address.h>\n" + "> > +#include <linux/of_irq.h>\n" + "> > +#include <linux/slab.h>\n" + "> > +\n" + "> > +#define INTC_MASK\t0x0\n" + "> > +#define INTC_EOI\t0x20\n" + "> > +\n" + "> > +struct mtk_intc_chip_data {\n" + "> > +\tchar *name;\n" + "> > +\tstruct irq_chip chip;\n" + "> \n" + "> There is no need to embed a full struct irqchip per controller, see \n" + "> below.\n" "\n" - "Why the restrictive type? Why isn't unsigned int good enough?\n" + "We want to distinguish which controller the device interrupts are belong to\n" + "by \"cat /proc/interrupts\".\n" + "And if all the controller share the same struct, the name field will be the\n" + "same.\n" + "Do you have suggestion for this?\n" "\n" - "> +\tu16 val, mask;\n" - "> +\n" - "> +\tmask = 1 << (index % 16);\n" - "> +\tval = readw_relaxed(base + offset + (index / 16) * 4) | mask;\n" - "> +\twritew_relaxed(val, base + offset + (index / 16) * 4);\n" + "> \n" + "> > +\tunsigned int irq_start, nr_irqs;\n" + "> > +\tvoid __iomem *base;\n" + "> > +\n" + "> };\n" + "> > +\n" + "> > +static void mtk_poke_irq(struct irq_data *d, u32 offset)\n" + "> \t> +{\n" + "> > +\tstruct mtk_intc_chip_data *cd = irq_data_get_irq_chip_data(d);\n" + "> > +\tvoid __iomem *base = cd->base;\n" + "> > +\tu8 index = (u8)irqd_to_hwirq(d);\n" + "> \n" + "> Why the restrictive type? Why isn't unsigned int good enough?\n" "\n" - "RMW without locking, you will end-up with corruption.\n" - "Please store the address calculation in a temporaty variable to make it\n" - "more readable\n" + "You're right, unsigned int is ok.\n" + "I'll fix it in patch v2.\n" "\n" - "> +}\n" - "> +\n" - "> +static void mtk_clear_irq(struct irq_data *d, u32 offset)\n" - "> +{\n" - "> +\tstruct mtk_intc_chip_data *cd = irq_data_get_irq_chip_data(d);\n" - "> +\tvoid __iomem *base = cd->base;\n" - "> +\tu8 index = (u8)irqd_to_hwirq(d);\n" - "> +\tu16 val, mask;\n" - "> +\n" - "> +\tmask = 1 << (index % 16);\n" - "> +\tval = readw_relaxed(base + offset + (index / 16) * 4) & ~mask;\n" - "> +\twritew_relaxed(val, base + offset + (index / 16) * 4);\n" + "> \n" + "> > +\tu16 val, mask;\n" + "> > +\n" + "> > +\tmask = 1 << (index % 16);\n" + "> > +\tval = readw_relaxed(base + offset + (index / 16) * 4) | mask;\n" + "> > +\twritew_relaxed(val, base + offset + (index / 16) * 4);\n" + "> \n" + "> RMW without locking, you will end-up with corruption.\n" + "> Please store the address calculation in a temporaty variable to make it\n" + "> more readable\n" + "> \n" "\n" - "Same comments.\n" + "Thanks for the comment, I will fix it in pacth v2.\n" "\n" - "> +}\n" - "> +\n" - "> +static void mtk_intc_mask_irq(struct irq_data *d)\n" - "> +{\n" - "> +\tmtk_poke_irq(d, INTC_MASK);\n" - "> +\tirq_chip_mask_parent(d);\n" - "> +}\n" - "> +\n" - "> +static void mtk_intc_unmask_irq(struct irq_data *d)\n" - "> +{\n" - "> +\tmtk_clear_irq(d, INTC_MASK);\n" - "> +\tirq_chip_unmask_parent(d);\n" - "> +}\n" - "> +\n" - "> +static void mtk_intc_eoi_irq(struct irq_data *d)\n" - "> +{\n" - "> +\tmtk_poke_irq(d, INTC_EOI);\n" - "> +\tirq_chip_eoi_parent(d);\n" - "> +}\n" - "> +\n" - "> +static struct irq_chip mtk_intc_chip = {\n" - "> +\t.irq_mask\t\t= mtk_intc_mask_irq,\n" - "> +\t.irq_unmask\t\t= mtk_intc_unmask_irq,\n" - "> +\t.irq_eoi\t\t= mtk_intc_eoi_irq,\n" - "> +\t.irq_get_irqchip_state\t= irq_chip_get_parent_state,\n" - "> +\t.irq_set_irqchip_state\t= irq_chip_set_parent_state,\n" - "> +\t.irq_set_affinity\t= irq_chip_set_affinity_parent,\n" - "> +\t.irq_set_vcpu_affinity\t= irq_chip_set_vcpu_affinity_parent,\n" + "> > +\n" + "> \t}\n" + "> > +\n" + "> > +static void mtk_clear_irq(struct irq_data *d, u32 offset)\n" + "> \t> +{\n" + "> > +\tstruct mtk_intc_chip_data *cd = irq_data_get_irq_chip_data(d);\n" + "> > +\tvoid __iomem *base = cd->base;\n" + "> > +\tu8 index = (u8)irqd_to_hwirq(d);\n" + "> > +\tu16 val, mask;\n" + "> > +\n" + "> > +\tmask = 1 << (index % 16);\n" + "> > +\tval = readw_relaxed(base + offset + (index / 16) * 4) & ~mask;\n" + "> > +\twritew_relaxed(val, base + offset + (index / 16) * 4);\n" + "> \n" + "> Same comments.\n" + "> \n" "\n" - "How about retrigger?\n" + "You're right, unsigned int is ok.\n" + "I'll fix it in patch v2.\n" "\n" - "> +\t.irq_set_type\t\t= irq_chip_set_type_parent,\n" - "> +\t.flags\t\t\t= IRQCHIP_SET_TYPE_MASKED |\n" - "> +\t\t\t\t IRQCHIP_SKIP_SET_WAKE |\n" - "> +\t\t\t\t IRQCHIP_MASK_ON_SUSPEND,\n" - "> +};\n" - "> +\n" - "> +static int mt58xx_intc_domain_translate(struct irq_domain *d,\n" - "> +\t\t\t\t\tstruct irq_fwspec *fwspec,\n" - "> +\t\t\t\t\tunsigned long *hwirq,\n" - "> +\t\t\t\t\tunsigned int *type)\n" - "> +{\n" - "> +\tif (is_of_node(fwspec->fwnode)) {\n" - "> +\t\tif (fwspec->param_count != 3)\n" - "> +\t\t\treturn -EINVAL;\n" - "> +\n" - "> +\t\t/* No PPI should point to this domain */\n" - "> +\t\tif (fwspec->param[0] != 0)\n" - "> +\t\t\treturn -EINVAL;\n" - "> +\n" - "> +\t\t*hwirq = fwspec->param[1];\n" - "> +\t\t*type = fwspec->param[2] & IRQ_TYPE_SENSE_MASK;\n" - "> +\t\treturn 0;\n" - "> +\t}\n" - "> +\n" - "> +\treturn -EINVAL;\n" - "> +}\n" - "> +\n" - "> +static int mt58xx_intc_domain_alloc(struct irq_domain *domain,\n" - "> unsigned int virq,\n" - "> +\t\t\t\t unsigned int nr_irqs, void *data)\n" - "> +{\n" - "> +\tint i;\n" - "> +\tirq_hw_number_t hwirq;\n" - "> +\tstruct irq_fwspec parent_fwspec, *fwspec = data;\n" - "> +\tstruct mtk_intc_chip_data *cd = (struct mtk_intc_chip_data\n" - "> *)domain->host_data;\n" - "> +\n" - "> +\t/* Not GIC compliant */\n" - "> +\tif (fwspec->param_count != 3)\n" - "> +\t\treturn -EINVAL;\n" - "> +\n" - "> +\t/* No PPI should point to this domain */\n" - "> +\tif (fwspec->param[0])\n" - "> +\t\treturn -EINVAL;\n" - "> +\n" - "> +\tif (fwspec->param[1] >= cd->nr_irqs)\n" - "> +\t\treturn -EINVAL;\n" - "> +\n" - "> +\thwirq = fwspec->param[1];\n" - "> +\tfor (i = 0; i < nr_irqs; i++)\n" - "> +\t\tirq_domain_set_hwirq_and_chip(domain, virq + i, hwirq + i,\n" - "> +\t\t\t\t\t &cd->chip,\n" - "> +\t\t\t\t\t domain->host_data);\n" - "> +\n" - "> +\tparent_fwspec = *fwspec;\n" - "> +\tparent_fwspec.fwnode = domain->parent->fwnode;\n" - "> +\tparent_fwspec.param[1] = cd->irq_start + hwirq;\n" - "> +\treturn irq_domain_alloc_irqs_parent(domain, virq, nr_irqs, \n" - "> &parent_fwspec);\n" - "> +}\n" - "> +\n" - "> +static const struct irq_domain_ops mt58xx_intc_domain_ops = {\n" - "> +\t.translate\t= mt58xx_intc_domain_translate,\n" - "> +\t.alloc\t\t= mt58xx_intc_domain_alloc,\n" - "> +\t.free\t\t= irq_domain_free_irqs_common,\n" - "> +};\n" - "> +\n" - "> +int __init\n" - "> +mt58xx_intc_of_init(struct device_node *dn, struct device_node \n" - "> *parent)\n" - "> +{\n" - "> +\tstatic int nr_intc;\n" - "> +\tstruct irq_domain *domain, *domain_parent;\n" - "> +\tstruct mtk_intc_chip_data *cd;\n" - "> +\tunsigned int irq_start, irq_end;\n" - "> +\n" - "> +\tdomain_parent = irq_find_host(parent);\n" - "> +\tif (!domain_parent) {\n" - "> +\t\tpr_err(\"mt58xx-intc: interrupt-parent not found\\n\");\n" - "> +\t\treturn -EINVAL;\n" - "> +\t}\n" - "> +\n" - "> +\tcd = kzalloc(sizeof(*cd), GFP_KERNEL);\n" - "> +\tif (!cd)\n" - "> +\t\treturn -ENOMEM;\n" - "> +\n" - "> +\tcd->chip = mtk_intc_chip;\n" - "> +\tif (of_property_read_u32_index(dn, \"mediatek,irqs-map-range\", 0,\n" - "> &irq_start) ||\n" - "> +\t of_property_read_u32_index(dn, \"mediatek,irqs-map-range\", 1, \n" - "> &irq_end)) {\n" - "> +\t\tkfree(cd);\n" - "> +\t\treturn -EINVAL;\n" - "> +\t}\n" - "> +\n" - "> +\tif (of_property_read_bool(dn, \"mediatek,intc-no-eoi\"))\n" - "> +\t\tcd->chip.irq_eoi = irq_chip_eoi_parent;\n" + "> > +\n" + "> \t}\n" + "> > +\n" + "> > +static void mtk_intc_mask_irq(struct irq_data *d)\n" + "> \t> +{\n" + "> > +\tmtk_poke_irq(d, INTC_MASK);\n" + "> > +\tirq_chip_mask_parent(d);\n" + "> > +\n" + "> \t}\n" + "> > +\n" + "> > +static void mtk_intc_unmask_irq(struct irq_data *d)\n" + "> \t> +{\n" + "> > +\tmtk_clear_irq(d, INTC_MASK);\n" + "> > +\tirq_chip_unmask_parent(d);\n" + "> > +\n" + "> \t}\n" + "> > +\n" + "> > +static void mtk_intc_eoi_irq(struct irq_data *d)\n" + "> \t> +{\n" + "> > +\tmtk_poke_irq(d, INTC_EOI);\n" + "> > +\tirq_chip_eoi_parent(d);\n" + "> > +\n" + "> \t}\n" + "> > +\n" + "> > +static struct irq_chip mtk_intc_chip = {\n" + "> > +\t.irq_mask\t\t= mtk_intc_mask_irq,\n" + "> > +\t.irq_unmask\t\t= mtk_intc_unmask_irq,\n" + "> > +\t.irq_eoi\t\t= mtk_intc_eoi_irq,\n" + "> > +\t.irq_get_irqchip_state\t= irq_chip_get_parent_state,\n" + "> > +\t.irq_set_irqchip_state\t= irq_chip_set_parent_state,\n" + "> > +\t.irq_set_affinity\t= irq_chip_set_affinity_parent,\n" + "> > +\t.irq_set_vcpu_affinity\t= irq_chip_set_vcpu_affinity_parent,\n" + "> \n" + "> How about retrigger?\n" + "> \n" "\n" - "No. Just add a flag to your chip data structure, and check for this\n" - "flag in your irq_eoi callback. Or provide two distinct irq_chip\n" - "structures that will only differ by the irq_eoi method.\n" + "What is retrigger means?\n" + "To be honest, I just try to direct all the irqchip ops implemented in \n" + "/drivers/irqchip/irq-gic.c to gic driver.\n" + "But \"irq_set_vcpu_affinity\" is not used in our projects now.\n" + "Should I remove \".irq_set_vcpu_affinity\" here?\n" "\n" - "> +\n" - "> +\tcd->irq_start = irq_start;\n" - "> +\tcd->nr_irqs = irq_end - irq_start + 1;\n" - "> +\tcd->chip.name = kasprintf(GFP_KERNEL, \"mt58xx-intc-%d\", nr_intc++);\n" + "> > +\t.irq_set_type\t\t= irq_chip_set_type_parent,\n" + "> > +\t.flags\t\t\t= IRQCHIP_SET_TYPE_MASKED |\n" + "> > +\t\t\t\t IRQCHIP_SKIP_SET_WAKE |\n" + "> > +\t\t\t\t IRQCHIP_MASK_ON_SUSPEND,\n" + "> > +\n" + "> };\n" + "> > +\n" + "> > +static int mt58xx_intc_domain_translate(struct irq_domain *d,\n" + "> > +\t\t\t\t\tstruct irq_fwspec *fwspec,\n" + "> > +\t\t\t\t\tunsigned long *hwirq,\n" + "> > +\t\t\t\t\tunsigned int *type)\n" + "> \t> +{\n" + "> \t\t> +\tif (is_of_node(fwspec->fwnode)) {\n" + "> > +\t\tif (fwspec->param_count != 3)\n" + "> > +\t\t\treturn -EINVAL;\n" + "> > +\n" + "> > +\t\t/* No PPI should point to this domain */\n" + "> > +\t\tif (fwspec->param[0] != 0)\n" + "> > +\t\t\treturn -EINVAL;\n" + "> > +\n" + "> > +\t\t*hwirq = fwspec->param[1];\n" + "> > +\t\t*type = fwspec->param[2] & IRQ_TYPE_SENSE_MASK;\n" + "> > +\t\treturn 0;\n" + "> > +\t\n" + "> \t\t}\n" + "> > +\n" + "> > +\treturn -EINVAL;\n" + "> > +\n" + "> \t}\n" + "> > +\n" + "> > +static int mt58xx_intc_domain_alloc(struct irq_domain *domain,\n" + "> > unsigned int virq,\n" + "> > +\t\t\t\t unsigned int nr_irqs, void *data)\n" + "> \t> +{\n" + "> > +\tint i;\n" + "> > +\tirq_hw_number_t hwirq;\n" + "> > +\tstruct irq_fwspec parent_fwspec, *fwspec = data;\n" + "> > +\tstruct mtk_intc_chip_data *cd = (struct mtk_intc_chip_data\n" + "> > *)domain->host_data;\n" + "> > +\n" + "> > +\t/* Not GIC compliant */\n" + "> > +\tif (fwspec->param_count != 3)\n" + "> > +\t\treturn -EINVAL;\n" + "> > +\n" + "> > +\t/* No PPI should point to this domain */\n" + "> > +\tif (fwspec->param[0])\n" + "> > +\t\treturn -EINVAL;\n" + "> > +\n" + "> > +\tif (fwspec->param[1] >= cd->nr_irqs)\n" + "> > +\t\treturn -EINVAL;\n" + "> > +\n" + "> > +\thwirq = fwspec->param[1];\n" + "> > +\tfor (i = 0; i < nr_irqs; i++)\n" + "> > +\t\tirq_domain_set_hwirq_and_chip(domain, virq + i, hwirq + i,\n" + "> > +\t\t\t\t\t &cd->chip,\n" + "> > +\t\t\t\t\t domain->host_data);\n" + "> > +\n" + "> > +\tparent_fwspec = *fwspec;\n" + "> > +\tparent_fwspec.fwnode = domain->parent->fwnode;\n" + "> > +\tparent_fwspec.param[1] = cd->irq_start + hwirq;\n" + "> > +\treturn irq_domain_alloc_irqs_parent(domain, virq, nr_irqs, \n" + "> > &parent_fwspec);\n" + "> > +\n" + "> \t}\n" + "> > +\n" + "> > +static const struct irq_domain_ops mt58xx_intc_domain_ops = {\n" + "> > +\t.translate\t= mt58xx_intc_domain_translate,\n" + "> > +\t.alloc\t\t= mt58xx_intc_domain_alloc,\n" + "> > +\t.free\t\t= irq_domain_free_irqs_common,\n" + "> > +\n" + "> };\n" + "> > +\n" + "> > +int __init\n" + "> > +mt58xx_intc_of_init(struct device_node *dn, struct device_node \n" + "> > *parent)\n" + "> > +{\n" + "> > +\tstatic int nr_intc;\n" + "> > +\tstruct irq_domain *domain, *domain_parent;\n" + "> > +\tstruct mtk_intc_chip_data *cd;\n" + "> > +\tunsigned int irq_start, irq_end;\n" + "> > +\n" + "> > +\tdomain_parent = irq_find_host(parent);\n" + "> > +\tif (!domain_parent) {\n" + "> > +\t\tpr_err(\"mt58xx-intc: interrupt-parent not found\\n\");\n" + "> > +\t\treturn -EINVAL;\n" + "> > +\t\n" + "> }\n" + "> > +\n" + "> > +\tcd = kzalloc(sizeof(*cd), GFP_KERNEL);\n" + "> > +\tif (!cd)\n" + "> > +\t\treturn -ENOMEM;\n" + "> > +\n" + "> > +\tcd->chip = mtk_intc_chip;\n" + "> > +\tif (of_property_read_u32_index(dn, \"mediatek,irqs-map-range\", 0,\n" + "> > &irq_start) ||\n" + "> > +\t of_property_read_u32_index(dn, \"mediatek,irqs-map-range\", 1, \n" + "> \t> &irq_end)) {\n" + "> > +\t\tkfree(cd);\n" + "> > +\t\treturn -EINVAL;\n" + "> > +\t\n" + "> }\n" + "> > +\n" + "> > +\tif (of_property_read_bool(dn, \"mediatek,intc-no-eoi\"))\n" + "> > +\t\tcd->chip.irq_eoi = irq_chip_eoi_parent;\n" + "> \n" + "> No. Just add a flag to your chip data structure, and check for this\n" + "> flag in your irq_eoi callback. Or provide two distinct irq_chip\n" + "> structures that will only differ by the irq_eoi method.\n" "\n" - "Neither. That's not useful, and is a waste of memory. Stick to constant\n" - "names in the irq_chip structure.\n" + "Thanks for the comment, I will modify it in patch v2.\n" "\n" - "> +\tif (!cd->chip.name) {\n" - "> +\t\tkfree(cd);\n" - "> +\t\treturn -ENOMEM;\n" - "> +\t}\n" - "> +\n" - "> +\tcd->base = of_iomap(dn, 0);\n" - "> +\tif (!cd->base) {\n" - "> +\t\tkfree(cd->chip.name);\n" - "> +\t\tkfree(cd);\n" - "> +\t\treturn -ENOMEM;\n" - "> +\t}\n" - "> +\n" - "> +\tdomain = irq_domain_add_hierarchy(domain_parent, 0, cd->nr_irqs,\n" - "> +\t\t\t\t\t dn, &mt58xx_intc_domain_ops, cd);\n" - "> +\tif (!domain) {\n" - "> +\t\tkfree(cd->chip.name);\n" - "> +\t\tiounmap(cd->base);\n" - "> +\t\tkfree(cd);\n" - "> +\t\treturn -ENOMEM;\n" - "> +\t}\n" - "> +\n" - "> +\treturn 0;\n" - "> +}\n" - "> +\n" - "> +IRQCHIP_DECLARE(mt58xx_intc, \"mediatek,mt58xx-intc\", \n" - "> mt58xx_intc_of_init);\n" + "> \n" + "> > +\n" + "> > +\tcd->irq_start = irq_start;\n" + "> > +\tcd->nr_irqs = irq_end - irq_start + 1;\n" + "> > +\tcd->chip.name = kasprintf(GFP_KERNEL, \"mt58xx-intc-%d\", nr_intc++);\n" + "> \n" + "> Neither. That's not useful, and is a waste of memory. Stick to constant\n" + "> names in the irq_chip structure.\n" + "> \n" "\n" - "On a side note, the merge window has just opened. Please refrain from\n" - "reposting this until -rc1.\n" + "Actually we have multiple irq controller in our SoCs.\n" + "And if we use the constant names in irq_chip structure, the information in\n" + "\"/proc/interrupts\" will be hard to understand because all the irqchip name\n" + "is the same.\n" + "Do you have any suggestion for this?\n" "\n" - "Thanks,\n" + "> > +\tif (!cd->chip.name) {\n" + "> > +\t\tkfree(cd);\n" + "> > +\t\treturn -ENOMEM;\n" + "> > +\t\n" + "> }\n" + "> > +\n" + "> > +\tcd->base = of_iomap(dn, 0);\n" + "> > +\tif (!cd->base) {\n" + "> > +\t\tkfree(cd->chip.name);\n" + "> > +\t\tkfree(cd);\n" + "> > +\t\treturn -ENOMEM;\n" + "> > +\t\n" + "> }\n" + "> > +\n" + "> > +\tdomain = irq_domain_add_hierarchy(domain_parent, 0, cd->nr_irqs,\n" + "> > +\t\t\t\t\t dn, &mt58xx_intc_domain_ops, cd);\n" + "> > +\tif (!domain) {\n" + "> > +\t\tkfree(cd->chip.name);\n" + "> > +\t\tiounmap(cd->base);\n" + "> > +\t\tkfree(cd);\n" + "> > +\t\treturn -ENOMEM;\n" + "> > +\t\n" + "> }\n" + "> > +\n" + "> > +\treturn 0;\n" + "> > +\n" + "> }\n" + "> > +\n" + "> > +IRQCHIP_DECLARE(mt58xx_intc, \"mediatek,mt58xx-intc\", \n" + "> > mt58xx_intc_of_init);\n" + "> \n" + "> On a side note, the merge window has just opened. Please refrain from\n" + "> reposting this until -rc1.\n" "\n" - " M.\n" - "-- \n" - Jazz is not dead. It just smells funny... + "Got it, and thanks for your comments.\n" + I'll update the patch and post it after -rc1. -52ace849e3d022657f7001422fe6e02ef7eee1ecdb556732e78355f6cf44766f +82ede7b9d6531580e965b2ff4718d364c0ee4f6230a435b2dbac40e24007b977
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox