| Message ID | 20200819034231.20726-1-mark-pk.tsai@mediatek.com |
|---|---|
| Headers | show |
| Series | Add MStar interrupt controller support | expand |
On 2020-08-19 04:42, Mark-PK Tsai wrote: > Add MStar 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-mst-intc.c | 195 +++++++++++++++++++++++++++++++++ > 3 files changed, 203 insertions(+) > create mode 100644 drivers/irqchip/irq-mst-intc.c > > diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig > index bb70b7177f94..c3a9d880a4ea 100644 > --- a/drivers/irqchip/Kconfig > +++ b/drivers/irqchip/Kconfig > @@ -571,4 +571,11 @@ config LOONGSON_PCH_MSI > help > Support for the Loongson PCH MSI Controller. > > +config MST_IRQ > + bool "MStar Interrupt Controller" > + select IRQ_DOMAIN > + select IRQ_DOMAIN_HIERARCHY > + help > + Support MStar Interrupt Controller. What selects it? Can it have a default for the relevant platforms? > + > endmenu > diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile > index 133f9c45744a..e2688a62403e 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_MST_IRQ) += irq-mst-intc.o > diff --git a/drivers/irqchip/irq-mst-intc.c > b/drivers/irqchip/irq-mst-intc.c > new file mode 100644 > index 000000000000..38d567741860 > --- /dev/null > +++ b/drivers/irqchip/irq-mst-intc.c > @@ -0,0 +1,195 @@ > +// 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> > +#include <linux/spinlock.h> > + > +#define INTC_MASK 0x0 > +#define INTC_EOI 0x20 > + > +struct mst_intc_chip_data { > + raw_spinlock_t lock; > + unsigned int irq_start, nr_irqs; > + void __iomem *base; > + bool no_eoi; nit: please align the fields of the data structure on a vertical line. > +}; > + > +static void mst_poke_irq(struct irq_data *d, u32 offset) Given that this is supposed to be the opposite of "clear", this should be mst_set_irq(). > +{ > + irq_hw_number_t hwirq = irqd_to_hwirq(d); > + struct mst_intc_chip_data *cd = irq_data_get_irq_chip_data(d); > + u16 val, mask; > + > + mask = 1 << (hwirq % 16); > + offset += (hwirq / 16) * 4; > + > + raw_spin_lock(&cd->lock); > + val = readw_relaxed(cd->base + offset) | mask; > + writew_relaxed(val, cd->base + offset); > + raw_spin_unlock(&cd->lock); Small improvement, but you still miss the fact that a masking operation can also occur from an interrupt handler, and this will cause a deadlock. You need to disable interrupts as well. > +} > + > +static void mst_clear_irq(struct irq_data *d, u32 offset) > +{ > + irq_hw_number_t hwirq = irqd_to_hwirq(d); > + struct mst_intc_chip_data *cd = irq_data_get_irq_chip_data(d); > + u16 val, mask; > + > + mask = 1 << (hwirq % 16); > + offset += (hwirq / 16) * 4; > + > + raw_spin_lock(&cd->lock); > + val = readw_relaxed(cd->base + offset) & ~mask; > + writew_relaxed(val, cd->base + offset); > + raw_spin_unlock(&cd->lock); > +} > + > +static void mst_intc_mask_irq(struct irq_data *d) > +{ > + mst_poke_irq(d, INTC_MASK); > + irq_chip_mask_parent(d); > +} > + > +static void mst_intc_unmask_irq(struct irq_data *d) > +{ > + mst_clear_irq(d, INTC_MASK); > + irq_chip_unmask_parent(d); > +} > + > +static void mst_intc_eoi_irq(struct irq_data *d) > +{ > + struct mst_intc_chip_data *cd = irq_data_get_irq_chip_data(d); > + > + if (!cd->no_eoi) > + mst_poke_irq(d, INTC_EOI); > + > + irq_chip_eoi_parent(d); > +} > + > +static struct irq_chip mst_intc_chip = { > + .name = "mst-intc", > + .irq_mask = mst_intc_mask_irq, > + .irq_unmask = mst_intc_unmask_irq, > + .irq_eoi = mst_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, > + .irq_set_type = irq_chip_set_type_parent, > + .irq_retrigger = irq_chip_retrigger_hierarchy, > + .flags = IRQCHIP_SET_TYPE_MASKED | > + IRQCHIP_SKIP_SET_WAKE | > + IRQCHIP_MASK_ON_SUSPEND, > +}; > + > +static int mst_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 mst_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 mst_intc_chip_data *cd = (struct mst_intc_chip_data > *)domain->host_data; No cast necessary here. > + > + /* 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) This condition is bogus, as it doesn't take into account the nr_irqs parameter. > + return -EINVAL; > + > + hwirq = fwspec->param[1]; > + for (i = 0; i < nr_irqs; i++) > + irq_domain_set_hwirq_and_chip(domain, virq + i, hwirq + i, > + &mst_intc_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 mst_intc_domain_ops = { > + .translate = mst_intc_domain_translate, > + .alloc = mst_intc_domain_alloc, > + .free = irq_domain_free_irqs_common, > +}; > + > +int __init > +mst_intc_of_init(struct device_node *dn, struct device_node *parent) > +{ > + struct irq_domain *domain, *domain_parent; > + struct mst_intc_chip_data *cd; > + unsigned int irq_start, irq_end; unsigned int != u32. > + > + domain_parent = irq_find_host(parent); > + if (!domain_parent) { > + pr_err("mst-intc: interrupt-parent not found\n"); > + return -EINVAL; > + } > + > + if (of_property_read_u32_index(dn, "mstar,irqs-map-range", 0, > &irq_start) || > + of_property_read_u32_index(dn, "mstar,irqs-map-range", 1, > &irq_end)) > + return -EINVAL; > + > + cd = kzalloc(sizeof(*cd), GFP_KERNEL); > + if (!cd) > + return -ENOMEM; > + > + cd->base = of_iomap(dn, 0); > + if (!cd->base) { > + kfree(cd); > + return -ENOMEM; > + } > + > + cd->no_eoi = of_property_read_bool(dn, "mstar,intc-no-eoi"); > + raw_spin_lock_init(&cd->lock); > + cd->irq_start = irq_start; > + cd->nr_irqs = irq_end - irq_start + 1; > + domain = irq_domain_add_hierarchy(domain_parent, 0, cd->nr_irqs, dn, > + &mst_intc_domain_ops, cd); > + if (!domain) { > + iounmap(cd->base); > + kfree(cd); > + return -ENOMEM; > + } > + > + return 0; > +} > + > +IRQCHIP_DECLARE(mst_intc, "mstar,mst-intc", mst_intc_of_init); Thanks, M.
From: Marc Zyngier <maz@kernel.org> > > + > > +static int mst_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 mst_intc_chip_data *cd = (struct mst_intc_chip_data > > *)domain->host_data; > > No cast necessary here. > > > + > > + /* 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) > > This condition is bogus, as it doesn't take into account the nr_irqs > parameter. > The hwirq number need to be in the irq map range. (property: mstar,irqs-map-range) If it's not, it must be incorrect configuration. So how about use the condition as following? if (hwirq >= cd->nr_irqs) return -EINVAL; > > + return -EINVAL; > > + > > + hwirq = fwspec->param[1]; > > + for (i = 0; i < nr_irqs; i++) > > + irq_domain_set_hwirq_and_chip(domain, virq + i, hwirq + i, > > + &mst_intc_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 mst_intc_domain_ops = { > > + .translate = mst_intc_domain_translate, > > + .alloc = mst_intc_domain_alloc, > > + .free = irq_domain_free_irqs_common, > > +}; > > + > > +int __init > > +mst_intc_of_init(struct device_node *dn, struct device_node *parent) > > +{ > > + struct irq_domain *domain, *domain_parent; > > + struct mst_intc_chip_data *cd; > > + unsigned int irq_start, irq_end; > > unsigned int != u32. > > > + > > + domain_parent = irq_find_host(parent); > > + if (!domain_parent) { > > + pr_err("mst-intc: interrupt-parent not found\n"); > > + return -EINVAL; > > + } > > + > > + if (of_property_read_u32_index(dn, "mstar,irqs-map-range", 0, > > &irq_start) || > > + of_property_read_u32_index(dn, "mstar,irqs-map-range", 1, > > &irq_end)) > > + return -EINVAL; > > + > > + cd = kzalloc(sizeof(*cd), GFP_KERNEL); > > + if (!cd) > > + return -ENOMEM; > > + > > + cd->base = of_iomap(dn, 0); > > + if (!cd->base) { > > + kfree(cd); > > + return -ENOMEM; > > + } > > + > > + cd->no_eoi = of_property_read_bool(dn, "mstar,intc-no-eoi"); > > + raw_spin_lock_init(&cd->lock); > > + cd->irq_start = irq_start; > > + cd->nr_irqs = irq_end - irq_start + 1; > > + domain = irq_domain_add_hierarchy(domain_parent, 0, cd->nr_irqs, dn, > > + &mst_intc_domain_ops, cd); > > + if (!domain) { > > + iounmap(cd->base); > > + kfree(cd); > > + return -ENOMEM; > > + } > > + > > + return 0; > > +} > > + > > +IRQCHIP_DECLARE(mst_intc, "mstar,mst-intc", mst_intc_of_init); >
On 2020-08-19 14:31, Mark-PK Tsai wrote: > From: Marc Zyngier <maz@kernel.org> > >> > + >> > +static int mst_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 mst_intc_chip_data *cd = (struct mst_intc_chip_data >> > *)domain->host_data; >> >> No cast necessary here. >> >> > + >> > + /* 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) >> >> This condition is bogus, as it doesn't take into account the nr_irqs >> parameter. >> > > > The hwirq number need to be in the irq map range. (property: > mstar,irqs-map-range) > If it's not, it must be incorrect configuration. I agree. And since you are checking whether the configuration is correct, it'd better be completely correct. > So how about use the condition as following? > > if (hwirq >= cd->nr_irqs) > return -EINVAL; Again, this says nothing of the validity of (hwirq + nr_irqs - 1)... > >> > + return -EINVAL; >> > + >> > + hwirq = fwspec->param[1]; >> > + for (i = 0; i < nr_irqs; i++) >> > + irq_domain_set_hwirq_and_chip(domain, virq + i, hwirq + i, >> > + &mst_intc_chip, >> > + domain->host_data); ... which you are using here. M.
From: Marc Zyngier <maz@kernel.org> >On 2020-08-19 14:31, Mark-PK Tsai wrote: >> From: Marc Zyngier <maz@kernel.org> >> >>> > + >>> > +static int mst_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 mst_intc_chip_data *cd = (struct mst_intc_chip_data >>> > *)domain->host_data; >>> >>> No cast necessary here. >>> >>> > + >>> > + /* 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) >>> >>> This condition is bogus, as it doesn't take into account the nr_irqs >>> parameter. >>> >> >> >> The hwirq number need to be in the irq map range. (property: >> mstar,irqs-map-range) >> If it's not, it must be incorrect configuration. > >I agree. And since you are checking whether the configuration is >correct, >it'd better be completely correct. > >> So how about use the condition as following? >> >> if (hwirq >= cd->nr_irqs) >> return -EINVAL; > >Again, this says nothing of the validity of (hwirq + nr_irqs - 1)... > How about move this to mst_intc_domain_translate? Then all the irq_fwspec point to domain mst_intc should be valid. The mst_intc_domain_translate will be as following: static int mst_intc_domain_translate(struct irq_domain *d, struct irq_fwspec *fwspec, unsigned long *hwirq, unsigned int *type) { struct mst_intc_chip_data *cd = d->host_data; 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; if (fwspec->param[1] >= cd->nr_irqs) return -EINVAL; *hwirq = fwspec->param[1]; *type = fwspec->param[2] & IRQ_TYPE_SENSE_MASK; return 0; } return -EINVAL; } >> >>> > + return -EINVAL; >>> > + >>> > + hwirq = fwspec->param[1]; >>> > + for (i = 0; i < nr_irqs; i++) >>> > + irq_domain_set_hwirq_and_chip(domain, virq + i, hwirq + i, >>> > + &mst_intc_chip, >>> > + domain->host_data);
On 2020-08-19 15:55, Mark-PK Tsai wrote: > From: Marc Zyngier <maz@kernel.org> > >> On 2020-08-19 14:31, Mark-PK Tsai wrote: >>> From: Marc Zyngier <maz@kernel.org> >>> >>>> > + >>>> > +static int mst_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 mst_intc_chip_data *cd = (struct mst_intc_chip_data >>>> > *)domain->host_data; >>>> >>>> No cast necessary here. >>>> >>>> > + >>>> > + /* 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) >>>> >>>> This condition is bogus, as it doesn't take into account the nr_irqs >>>> parameter. >>>> >>> >>> >>> The hwirq number need to be in the irq map range. (property: >>> mstar,irqs-map-range) >>> If it's not, it must be incorrect configuration. >> >> I agree. And since you are checking whether the configuration is >> correct, >> it'd better be completely correct. >> >>> So how about use the condition as following? >>> >>> if (hwirq >= cd->nr_irqs) >>> return -EINVAL; >> >> Again, this says nothing of the validity of (hwirq + nr_irqs - 1)... >> > > How about move this to mst_intc_domain_translate? Then all the > irq_fwspec > point to domain mst_intc should be valid. > > The mst_intc_domain_translate will be as following: > > static int mst_intc_domain_translate(struct irq_domain *d, > struct irq_fwspec *fwspec, > unsigned long *hwirq, > unsigned int *type) > { > struct mst_intc_chip_data *cd = d->host_data; > > 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; > > if (fwspec->param[1] >= cd->nr_irqs) > return -EINVAL; > > *hwirq = fwspec->param[1]; > *type = fwspec->param[2] & IRQ_TYPE_SENSE_MASK; > return 0; > } > > return -EINVAL; > } It would make more sense. M.
Hi Mark-PK, Marc I'm not sure this will be the final version but I'm going to try to integrate this with my current MStar/SigmaStar tree over the weekend and then I guess I can give this a tested-by? Assuming this version or the next is acceptable can I just follow up with a small patch to add the instances I need in my dtsi or should I wait until it's merged before doing that? Thanks, Daniel On Thu, 20 Aug 2020 at 00:38, Mark-PK Tsai <mark-pk.tsai@mediatek.com> wrote: > > Add MStar interrupt controller support using hierarchy irq > domain. > > Signed-off-by: Mark-PK Tsai <mark-pk.tsai@mediatek.com> > --- > drivers/irqchip/Kconfig | 8 ++ > drivers/irqchip/Makefile | 1 + > drivers/irqchip/irq-mst-intc.c | 199 +++++++++++++++++++++++++++++++++ > 3 files changed, 208 insertions(+) > create mode 100644 drivers/irqchip/irq-mst-intc.c > > diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig > index bb70b7177f94..0b5ae5fa0d3c 100644 > --- a/drivers/irqchip/Kconfig > +++ b/drivers/irqchip/Kconfig > @@ -571,4 +571,12 @@ config LOONGSON_PCH_MSI > help > Support for the Loongson PCH MSI Controller. > > +config MST_IRQ > + bool "MStar Interrupt Controller" > + default ARCH_MEDIATEK > + select IRQ_DOMAIN > + select IRQ_DOMAIN_HIERARCHY > + help > + Support MStar Interrupt Controller. > + > endmenu > diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile > index 133f9c45744a..e2688a62403e 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_MST_IRQ) += irq-mst-intc.o > diff --git a/drivers/irqchip/irq-mst-intc.c b/drivers/irqchip/irq-mst-intc.c > new file mode 100644 > index 000000000000..4be077591898 > --- /dev/null > +++ b/drivers/irqchip/irq-mst-intc.c > @@ -0,0 +1,199 @@ > +// 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> > +#include <linux/spinlock.h> > + > +#define INTC_MASK 0x0 > +#define INTC_EOI 0x20 > + > +struct mst_intc_chip_data { > + raw_spinlock_t lock; > + unsigned int irq_start, nr_irqs; > + void __iomem *base; > + bool no_eoi; > +}; > + > +static void mst_set_irq(struct irq_data *d, u32 offset) > +{ > + irq_hw_number_t hwirq = irqd_to_hwirq(d); > + struct mst_intc_chip_data *cd = irq_data_get_irq_chip_data(d); > + u16 val, mask; > + unsigned long flags; > + > + mask = 1 << (hwirq % 16); > + offset += (hwirq / 16) * 4; > + > + raw_spin_lock_irqsave(&cd->lock, flags); > + val = readw_relaxed(cd->base + offset) | mask; > + writew_relaxed(val, cd->base + offset); > + raw_spin_unlock_irqrestore(&cd->lock, flags); > +} > + > +static void mst_clear_irq(struct irq_data *d, u32 offset) > +{ > + irq_hw_number_t hwirq = irqd_to_hwirq(d); > + struct mst_intc_chip_data *cd = irq_data_get_irq_chip_data(d); > + u16 val, mask; > + unsigned long flags; > + > + mask = 1 << (hwirq % 16); > + offset += (hwirq / 16) * 4; > + > + raw_spin_lock_irqsave(&cd->lock, flags); > + val = readw_relaxed(cd->base + offset) & ~mask; > + writew_relaxed(val, cd->base + offset); > + raw_spin_unlock_irqrestore(&cd->lock, flags); > +} > + > +static void mst_intc_mask_irq(struct irq_data *d) > +{ > + mst_set_irq(d, INTC_MASK); > + irq_chip_mask_parent(d); > +} > + > +static void mst_intc_unmask_irq(struct irq_data *d) > +{ > + mst_clear_irq(d, INTC_MASK); > + irq_chip_unmask_parent(d); > +} > + > +static void mst_intc_eoi_irq(struct irq_data *d) > +{ > + struct mst_intc_chip_data *cd = irq_data_get_irq_chip_data(d); > + > + if (!cd->no_eoi) > + mst_set_irq(d, INTC_EOI); > + > + irq_chip_eoi_parent(d); > +} > + > +static struct irq_chip mst_intc_chip = { > + .name = "mst-intc", > + .irq_mask = mst_intc_mask_irq, > + .irq_unmask = mst_intc_unmask_irq, > + .irq_eoi = mst_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, > + .irq_set_type = irq_chip_set_type_parent, > + .irq_retrigger = irq_chip_retrigger_hierarchy, > + .flags = IRQCHIP_SET_TYPE_MASKED | > + IRQCHIP_SKIP_SET_WAKE | > + IRQCHIP_MASK_ON_SUSPEND, > +}; > + > +static int mst_intc_domain_translate(struct irq_domain *d, > + struct irq_fwspec *fwspec, > + unsigned long *hwirq, > + unsigned int *type) > +{ > + struct mst_intc_chip_data *cd = d->host_data; > + > + 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; > + > + if (fwspec->param[1] >= cd->nr_irqs) > + return -EINVAL; > + > + *hwirq = fwspec->param[1]; > + *type = fwspec->param[2] & IRQ_TYPE_SENSE_MASK; > + return 0; > + } > + > + return -EINVAL; > +} > + > +static int mst_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 mst_intc_chip_data *cd = 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; > + > + hwirq = fwspec->param[1]; > + for (i = 0; i < nr_irqs; i++) > + irq_domain_set_hwirq_and_chip(domain, virq + i, hwirq + i, > + &mst_intc_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 mst_intc_domain_ops = { > + .translate = mst_intc_domain_translate, > + .alloc = mst_intc_domain_alloc, > + .free = irq_domain_free_irqs_common, > +}; > + > +int __init > +mst_intc_of_init(struct device_node *dn, struct device_node *parent) > +{ > + struct irq_domain *domain, *domain_parent; > + struct mst_intc_chip_data *cd; > + u32 irq_start, irq_end; > + > + domain_parent = irq_find_host(parent); > + if (!domain_parent) { > + pr_err("mst-intc: interrupt-parent not found\n"); > + return -EINVAL; > + } > + > + if (of_property_read_u32_index(dn, "mstar,irqs-map-range", 0, &irq_start) || > + of_property_read_u32_index(dn, "mstar,irqs-map-range", 1, &irq_end)) > + return -EINVAL; > + > + cd = kzalloc(sizeof(*cd), GFP_KERNEL); > + if (!cd) > + return -ENOMEM; > + > + cd->base = of_iomap(dn, 0); > + if (!cd->base) { > + kfree(cd); > + return -ENOMEM; > + } > + > + cd->no_eoi = of_property_read_bool(dn, "mstar,intc-no-eoi"); > + raw_spin_lock_init(&cd->lock); > + cd->irq_start = irq_start; > + cd->nr_irqs = irq_end - irq_start + 1; > + domain = irq_domain_add_hierarchy(domain_parent, 0, cd->nr_irqs, dn, > + &mst_intc_domain_ops, cd); > + if (!domain) { > + iounmap(cd->base); > + kfree(cd); > + return -ENOMEM; > + } > + > + return 0; > +} > + > +IRQCHIP_DECLARE(mst_intc, "mstar,mst-intc", mst_intc_of_init); > -- > 2.18.0
On 2020-08-20 13:36, Daniel Palmer wrote: > Hi Mark-PK, Marc > > I'm not sure this will be the final version but I'm going to try to > integrate this with my current MStar/SigmaStar tree over the weekend > and then I guess I can give this a tested-by? That'd be good. > Assuming this version or the next is acceptable can I just follow up > with a small patch to add the instances I need in my dtsi or should I > wait until it's merged before doing that? No need to wait, although the platform-specific details should go via the arm-soc tree. I'm not going to review the new version before next week anyway (I'm making a point in reviewing any given series at most once a week). M.
Hi Mark-PK, On Thu, 20 Aug 2020 at 00:38, Mark-PK Tsai <mark-pk.tsai@mediatek.com> wrote: > > Add MStar interrupt controller support using hierarchy irq > domain. > > Signed-off-by: Mark-PK Tsai <mark-pk.tsai@mediatek.com> I've integrated this version into my MStar/SigmaStar tree and tested on an MStar MSC313E based board (BreadBee) and I'm happy to say it seems to be working: $ cat /proc/interrupts CPU0 17: 1219 GIC-0 29 Level arch_timer 18: 0 GIC-0 30 Level arch_timer 21: 0 GIC-0 42 Level arm-pmu 24: 0 mst-intc 44 Level 1f002400.rtc 30: 0 mst-intc 2 Level 1f006000.wdt 31: 0 mst-intc 0 Level 1f006040.timer 32: 0 mst-intc 1 Level 1f006080.timer 33: 0 mst-intc 12 Level 1f0060c0.timer 34: 0 mst-intc 40 Level 1f200400.bdma 35: 3977 mst-intc 41 Level 1f200400.bdma 37: 196 mst-intc 34 Level ttyS0 40: 0 mst-intc 30 Level soc:usbphy@0 <snip> So here's my tested by: Tested-by: Daniel Palmer <daniel@thingy.jp> I don't think your series contained an update to MAINTAINERS. If/when you add this could you add my address above as a reviewer so I'm in the loop if anyone makes changes to this going forward? Thanks, Daniel
From: Daniel Palmer <daniel@0x0f.com> >Hi Mark-PK, > >On Thu, 20 Aug 2020 at 00:38, Mark-PK Tsai <mark-pk.tsai@mediatek.com> wrote: >> >> Add MStar interrupt controller support using hierarchy irq >> domain. >> >> Signed-off-by: Mark-PK Tsai <mark-pk.tsai@mediatek.com> > >I've integrated this version into my MStar/SigmaStar tree and tested >on an MStar MSC313E >based board (BreadBee) and I'm happy to say it seems to be working: > >$ cat /proc/interrupts > CPU0 >17: 1219 GIC-0 29 Level arch_timer >18: 0 GIC-0 30 Level arch_timer >21: 0 GIC-0 42 Level arm-pmu >24: 0 mst-intc 44 Level 1f002400.rtc >30: 0 mst-intc 2 Level 1f006000.wdt >31: 0 mst-intc 0 Level 1f006040.timer >32: 0 mst-intc 1 Level 1f006080.timer >33: 0 mst-intc 12 Level 1f0060c0.timer >34: 0 mst-intc 40 Level 1f200400.bdma >35: 3977 mst-intc 41 Level 1f200400.bdma >37: 196 mst-intc 34 Level ttyS0 >40: 0 mst-intc 30 Level soc:usbphy@0 ><snip> > >So here's my tested by: > >Tested-by: Daniel Palmer <daniel@thingy.jp> > Thanks for your test. >I don't think your series contained an update to MAINTAINERS. >If/when you add this could you add my address above as a reviewer so >I'm in the loop if anyone makes changes to this going forward? > Sure, I will add your address in there. :) Can I just add a patch in this thread which only contain MAINTAINERS update?
From: Marc Zyngier <maz@kernel.org> >On 2020-08-20 13:36, Daniel Palmer wrote: >> Hi Mark-PK, Marc >> >> I'm not sure this will be the final version but I'm going to try to >> integrate this with my current MStar/SigmaStar tree over the weekend >> and then I guess I can give this a tested-by? > >That'd be good. > >> Assuming this version or the next is acceptable can I just follow up >> with a small patch to add the instances I need in my dtsi or should I >> wait until it's merged before doing that? > >No need to wait, although the platform-specific details should go >via the arm-soc tree. > >I'm not going to review the new version before next week anyway >(I'm making a point in reviewing any given series at most once >a week). > > M. >-- >Jazz is not dead. It just smells funny... I've post the patch v3[1] and the driver is same as v2. The difference is that I add the test-by label by Daniel and add an entry in MAINTAINERS for mst-intc. Please review it and let me know if you have any suggestions. Thanks. [1] https://lore.kernel.org/lkml/20200902063344.1852-2-mark-pk.tsai@mediatek.com/