| Message ID | 55692C32.2050209@huawei.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 30/05/15 04:19, majun (F) wrote: > This patch is applied to support the mbigen interrupt. > > Change log: > --For irq_mbigen.c using,move some struct and function definition > to a new head file arm-gic-its.h > --Add a irq_write_mbi_msg member for mbi interrupt using > --For mbi interrupt, the event id depends on the Hardware pin number on mbigen, > so, the its_get_event_id is changed to calclulate the event id of mbi interrupt > --For mbigen, the device id information need to be carried with msg. So, > its_irq_compose_msi_msg is changed. > --its_mask_msi_irq and its_unmaks_msi_irq are modifid for mbi interrupt using. > --before the irq_ack callback, check the irq_ack first(chip.c) > > > Signed-off-by: Ma Jun <majun258@huawei.com> > --- > drivers/irqchip/irq-gic-v3-its.c | 71 +++++++++++----------------------- > include/linux/irq.h | 1 + > include/linux/irqchip/arm-gic-its.h | 68 +++++++++++++++++++++++++++++++++ > kernel/irq/chip.c | 12 ++++-- > 4 files changed, 100 insertions(+), 52 deletions(-) > mode change 100644 => 100755 drivers/irqchip/irq-gic-v3-its.c > create mode 100755 include/linux/irqchip/arm-gic-its.h > > diff --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq-gic-v3-its.c > old mode 100644 > new mode 100755 > index 9687f8a..21c36bf > --- a/drivers/irqchip/irq-gic-v3-its.c > +++ b/drivers/irqchip/irq-gic-v3-its.c > @@ -22,6 +22,7 @@ > #include <linux/log2.h> > #include <linux/mm.h> > #include <linux/msi.h> > +#include <linux/mbi.h> > #include <linux/of.h> > #include <linux/of_address.h> > #include <linux/of_irq.h> > @@ -30,7 +31,7 @@ > #include <linux/percpu.h> > #include <linux/slab.h> > > -#include <linux/irqchip/arm-gic-v3.h> > +#include <linux/irqchip/arm-gic-its.h> > > #include <asm/cacheflush.h> > #include <asm/cputype.h> > @@ -42,36 +43,6 @@ > > #define RDIST_FLAGS_PROPBASE_NEEDS_FLUSHING (1 << 0) > > -/* > - * Collection structure - just an ID, and a redistributor address to > - * ping. We use one per CPU as a bag of interrupts assigned to this > - * CPU. > - */ > -struct its_collection { > - u64 target_address; > - u16 col_id; > -}; > - > -/* > - * The ITS structure - contains most of the infrastructure, with the > - * msi_controller, the command queue, the collections, and the list of > - * devices writing to it. > - */ > -struct its_node { > - raw_spinlock_t lock; > - struct list_head entry; > - struct msi_controller msi_chip; > - struct irq_domain *domain; > - void __iomem *base; > - unsigned long phys_base; > - struct its_cmd_block *cmd_base; > - struct its_cmd_block *cmd_write; > - void *tables[GITS_BASER_NR_REGS]; > - struct its_collection *collections; > - struct list_head its_device_list; > - u64 flags; > - u32 ite_size; > -}; > > #define ITS_ITT_ALIGN SZ_256 > > @@ -79,17 +50,6 @@ struct its_node { > * The ITS view of a device - belongs to an ITS, a collection, owns an > * interrupt translation table, and a list of interrupts. > */ > -struct its_device { > - struct list_head entry; > - struct its_node *its; > - struct its_collection *collection; > - void *itt; > - unsigned long *lpi_map; > - irq_hw_number_t lpi_base; > - int nr_lpis; > - u32 nr_ites; > - u32 device_id; > -}; As I said before, there is no way an external driver is going to touch these. These structure are private to the ITS, and are definitely going to stay that way. > > static LIST_HEAD(its_nodes); > static DEFINE_SPINLOCK(its_lock); > @@ -528,7 +488,11 @@ static void its_send_invall(struct its_node *its, struct its_collection *col) > static inline u32 its_get_event_id(struct irq_data *d) > { > struct its_device *its_dev = irq_data_get_irq_chip_data(d); > - return d->hwirq - its_dev->lpi_base; > + > + if (!is_mbigen_domain(d)) > + return d->hwirq - its_dev->lpi_base; > + else > + return get_mbi_offset(d->irq); > } So on top of exposing the guts of the ITS code to the outside world, you are creating a compile-time dependency between the ITS and your MBI thing. No way. > > static void lpi_set_config(struct irq_data *d, bool enable) > @@ -599,7 +563,13 @@ static void its_irq_compose_msi_msg(struct irq_data *d, struct msi_msg *msg) > > msg->address_lo = addr & ((1UL << 32) - 1); > msg->address_hi = addr >> 32; > - msg->data = its_get_event_id(d); > + > + /*for devices connect to mbigen, device id > + *information is needed*/ > + if (!is_mbigen_domain(d)) > + msg->data = its_get_event_id(d); > + else > + msg->data = (its_dev->device_id << 16) | its_get_event_id(d); > } > > static struct irq_chip its_irq_chip = { > @@ -613,13 +583,15 @@ static struct irq_chip its_irq_chip = { > > static void its_mask_msi_irq(struct irq_data *d) > { > - pci_msi_mask_irq(d); > + if (!is_mbigen_domain(d)) > + pci_msi_mask_irq(d); > irq_chip_mask_parent(d); > } > > static void its_unmask_msi_irq(struct irq_data *d) > { > - pci_msi_unmask_irq(d); > + if (!is_mbigen_domain(d)) > + pci_msi_unmask_irq(d); > irq_chip_unmask_parent(d); > } > > @@ -629,6 +601,8 @@ static struct irq_chip its_msi_irq_chip = { > .irq_mask = its_mask_msi_irq, > .irq_eoi = irq_chip_eoi_parent, > .irq_write_msi_msg = pci_msi_domain_write_msg, > + .irq_write_mbi_msg = mbigen_write_msg, > + .irq_ack = irq_chip_ack_parent, > }; > Do you realize that you're hacking into code that is PCI specific, and that most of your changes could be solved elegantly by just having an MBI specific irqchip/domain? > /* > @@ -721,6 +695,7 @@ static void its_lpi_free(unsigned long *bitmap, int base, int nr_ids) > > for (lpi = base; lpi < (base + nr_ids); lpi += IRQS_PER_CHUNK) { > int chunk = its_lpi_to_chunk(lpi); > + > BUG_ON(chunk > lpi_chunks); > if (test_bit(chunk, lpi_bitmap)) { > clear_bit(chunk, lpi_bitmap); > @@ -1067,7 +1042,7 @@ static void its_cpu_init_collection(void) > spin_unlock(&its_lock); > } > > -static struct its_device *its_find_device(struct its_node *its, u32 dev_id) > +struct its_device *its_find_device(struct its_node *its, u32 dev_id) > { > struct its_device *its_dev = NULL, *tmp; > unsigned long flags; > @@ -1086,7 +1061,7 @@ static struct its_device *its_find_device(struct its_node *its, u32 dev_id) > return its_dev; > } > > -static struct its_device *its_create_device(struct its_node *its, u32 dev_id, > +struct its_device *its_create_device(struct its_node *its, u32 dev_id, > int nvecs) > { > struct its_device *dev; > diff --git a/include/linux/irq.h b/include/linux/irq.h > index 62c6901..9a745a8 100644 > --- a/include/linux/irq.h > +++ b/include/linux/irq.h > @@ -365,6 +365,7 @@ struct irq_chip { > > void (*irq_compose_msi_msg)(struct irq_data *data, struct msi_msg *msg); > void (*irq_write_msi_msg)(struct irq_data *data, struct msi_msg *msg); > + void (*irq_write_mbi_msg)(struct irq_data *irq_data, struct msi_msg *msg); > > int (*irq_get_irqchip_state)(struct irq_data *data, enum irqchip_irq_state which, bool *state); > int (*irq_set_irqchip_state)(struct irq_data *data, enum irqchip_irq_state which, bool state); > diff --git a/include/linux/irqchip/arm-gic-its.h b/include/linux/irqchip/arm-gic-its.h > new file mode 100755 > index 0000000..5677114 > --- /dev/null > +++ b/include/linux/irqchip/arm-gic-its.h > @@ -0,0 +1,68 @@ > +/* > + * Copyright (C) 2013, 2014 ARM Limited, All Rights Reserved. > + * Author: Marc Zyngier <marc.zyngier@arm.com> > + * > + * > + * This program is free software; you can redistribute it and/or modify > + * it under the terms of the GNU General Public License version 2 as > + * published by the Free Software Foundation. > + * > + * This program is distributed in the hope that it will be useful, > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > + * GNU General Public License for more details. > + * > + * You should have received a copy of the GNU General Public License > + * along with this program. If not, see <http://www.gnu.org/licenses/>. > + */ > +#ifndef __LINUX_IRQCHIP_ARM_GIC_ITS_H > +#define __LINUX_IRQCHIP_ARM_GIC_ITS_H > + > +#include <linux/irqchip/arm-gic-v3.h> > + > +struct its_device { > + struct list_head entry; > + struct its_node *its; > + struct its_collection *collection; > + void *itt; > + unsigned long *lpi_map; > + irq_hw_number_t lpi_base; > + int nr_lpis; > + u32 nr_ites; > + u32 device_id; > +}; > +/* > + * Collection structure - just an ID, and a redistributor address to > + * ping. We use one per CPU as a bag of interrupts assigned to this > + * CPU. > + */ > +struct its_collection { > + u64 target_address; > + u16 col_id; > +}; > + > +/* > + * The ITS structure - contains most of the infrastructure, with the > + * msi_controller, the command queue, the collections, and the list of > + * devices writing to it. > + */ > +struct its_node { > + raw_spinlock_t lock; > + struct list_head entry; > + struct msi_controller msi_chip; > + struct irq_domain *domain; > + void __iomem *base; > + unsigned long phys_base; > + struct its_cmd_block *cmd_base; > + struct its_cmd_block *cmd_write; > + void *tables[GITS_BASER_NR_REGS]; > + struct its_collection *collections; > + struct list_head its_device_list; > + u64 flags; > + u32 ite_size; > +}; > + > +struct its_device *its_find_device(struct its_node *its, u32 dev_id); > +struct its_device *its_create_device(struct its_node *its, u32 dev_id, > + int nvecs); > +#endif > diff --git a/kernel/irq/chip.c b/kernel/irq/chip.c > index eb9a4ea..3020efc 100644 > --- a/kernel/irq/chip.c > +++ b/kernel/irq/chip.c > @@ -882,7 +882,8 @@ void irq_cpu_offline(void) > void irq_chip_ack_parent(struct irq_data *data) > { > data = data->parent_data; > - data->chip->irq_ack(data); > + if (data->chip->irq_ack) > + data->chip->irq_ack(data); > } > > /** > @@ -892,7 +893,8 @@ void irq_chip_ack_parent(struct irq_data *data) > void irq_chip_mask_parent(struct irq_data *data) > { > data = data->parent_data; > - data->chip->irq_mask(data); > + if (data->chip->irq_mask) > + data->chip->irq_mask(data); > } > > /** > @@ -902,7 +904,8 @@ void irq_chip_mask_parent(struct irq_data *data) > void irq_chip_unmask_parent(struct irq_data *data) > { > data = data->parent_data; > - data->chip->irq_unmask(data); > + if (data->chip->irq_unmask) > + data->chip->irq_unmask(data); > } > > /** > @@ -912,7 +915,8 @@ void irq_chip_unmask_parent(struct irq_data *data) > void irq_chip_eoi_parent(struct irq_data *data) > { > data = data->parent_data; > - data->chip->irq_eoi(data); > + if (data->chip->irq_eoi) > + data->chip->irq_eoi(data); > } > > /** > Like I said in patch #1, you need to come up with a different approach. Use the LPI layer of the ITS as the parent for your MBI, stop messing with the PCI stuff, stop poking the ITS internals. Thanks, M.
Hi Marc: Thanks for your review. Accroding to my initial scheme, mbigen and pci devices uses the same parent domain--MSI domain. But accroding to your opinion, it seems use the ITS domain as parent domain of Mbigne is a better way. Am i right ? Thanks Ma Jun ? 2015/6/1 17:13, Marc Zyngier ??: > Like I said in patch #1, you need to come up with a different approach. > Use the LPI layer of the ITS as the parent for your MBI, stop messing > with the PCI stuff, stop poking the ITS internals. > > Thanks, > > M.
On 02/06/15 12:34, majun (F) wrote: > Hi Marc: Thanks for your review. > > Accroding to my initial scheme, mbigen and pci devices uses the same > parent domain--MSI domain. > > But accroding to your opinion, it seems use the ITS domain as parent > domain of Mbigne is a better way. Am i right ? Let's face it, your MBIGEN is not a PCI device at all. It is something else entirely, and you're just adding complexity to a subsystem that really doesn't need it. So why would you hack the PCI domain implementation at all? The kernel now has the right level of abstraction, where you can provide interrupts to different implementations of MSIs. Even better, you can base your MBIGEN domain on the core code that lives in kernel/irq/msi.c, just like PCI does (drivers/pci/msi.c). You will end up with something like: PCI/MSI --> ITS --> GIC MBIGEN _/ where both domains can coexist happily, and still share a common code base. This shouldn't require any hack either. The only possible snag is to be able to obtain a pointer to the ITS domain (there is currently no provision for that), but that should be easily solved. My point here is that if you need to do touch the core code, you need to be able to justify it. So far, I haven't seen anything in your HW that would require the kind of redesign you've posted. Thanks, M.
diff --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq-gic-v3-its.c old mode 100644 new mode 100755 index 9687f8a..21c36bf --- a/drivers/irqchip/irq-gic-v3-its.c +++ b/drivers/irqchip/irq-gic-v3-its.c @@ -22,6 +22,7 @@ #include <linux/log2.h> #include <linux/mm.h> #include <linux/msi.h> +#include <linux/mbi.h> #include <linux/of.h> #include <linux/of_address.h> #include <linux/of_irq.h> @@ -30,7 +31,7 @@ #include <linux/percpu.h> #include <linux/slab.h> -#include <linux/irqchip/arm-gic-v3.h> +#include <linux/irqchip/arm-gic-its.h> #include <asm/cacheflush.h> #include <asm/cputype.h> @@ -42,36 +43,6 @@ #define RDIST_FLAGS_PROPBASE_NEEDS_FLUSHING (1 << 0) -/* - * Collection structure - just an ID, and a redistributor address to - * ping. We use one per CPU as a bag of interrupts assigned to this - * CPU. - */ -struct its_collection { - u64 target_address; - u16 col_id; -}; - -/* - * The ITS structure - contains most of the infrastructure, with the - * msi_controller, the command queue, the collections, and the list of - * devices writing to it. - */ -struct its_node { - raw_spinlock_t lock; - struct list_head entry; - struct msi_controller msi_chip; - struct irq_domain *domain; - void __iomem *base; - unsigned long phys_base; - struct its_cmd_block *cmd_base; - struct its_cmd_block *cmd_write; - void *tables[GITS_BASER_NR_REGS]; - struct its_collection *collections; - struct list_head its_device_list; - u64 flags; - u32 ite_size; -}; #define ITS_ITT_ALIGN SZ_256 @@ -79,17 +50,6 @@ struct its_node { * The ITS view of a device - belongs to an ITS, a collection, owns an * interrupt translation table, and a list of interrupts. */ -struct its_device { - struct list_head entry; - struct its_node *its; - struct its_collection *collection; - void *itt; - unsigned long *lpi_map; - irq_hw_number_t lpi_base; - int nr_lpis; - u32 nr_ites; - u32 device_id; -}; static LIST_HEAD(its_nodes); static DEFINE_SPINLOCK(its_lock); @@ -528,7 +488,11 @@ static void its_send_invall(struct its_node *its, struct its_collection *col) static inline u32 its_get_event_id(struct irq_data *d) { struct its_device *its_dev = irq_data_get_irq_chip_data(d); - return d->hwirq - its_dev->lpi_base; + + if (!is_mbigen_domain(d)) + return d->hwirq - its_dev->lpi_base; + else + return get_mbi_offset(d->irq); } static void lpi_set_config(struct irq_data *d, bool enable) @@ -599,7 +563,13 @@ static void its_irq_compose_msi_msg(struct irq_data *d, struct msi_msg *msg) msg->address_lo = addr & ((1UL << 32) - 1); msg->address_hi = addr >> 32; - msg->data = its_get_event_id(d); + + /*for devices connect to mbigen, device id + *information is needed*/ + if (!is_mbigen_domain(d)) + msg->data = its_get_event_id(d); + else + msg->data = (its_dev->device_id << 16) | its_get_event_id(d); } static struct irq_chip its_irq_chip = { @@ -613,13 +583,15 @@ static struct irq_chip its_irq_chip = { static void its_mask_msi_irq(struct irq_data *d) { - pci_msi_mask_irq(d); + if (!is_mbigen_domain(d)) + pci_msi_mask_irq(d); irq_chip_mask_parent(d); } static void its_unmask_msi_irq(struct irq_data *d) { - pci_msi_unmask_irq(d); + if (!is_mbigen_domain(d)) + pci_msi_unmask_irq(d); irq_chip_unmask_parent(d); } @@ -629,6 +601,8 @@ static struct irq_chip its_msi_irq_chip = { .irq_mask = its_mask_msi_irq, .irq_eoi = irq_chip_eoi_parent, .irq_write_msi_msg = pci_msi_domain_write_msg, + .irq_write_mbi_msg = mbigen_write_msg, + .irq_ack = irq_chip_ack_parent, }; /* @@ -721,6 +695,7 @@ static void its_lpi_free(unsigned long *bitmap, int base, int nr_ids) for (lpi = base; lpi < (base + nr_ids); lpi += IRQS_PER_CHUNK) { int chunk = its_lpi_to_chunk(lpi); + BUG_ON(chunk > lpi_chunks); if (test_bit(chunk, lpi_bitmap)) { clear_bit(chunk, lpi_bitmap); @@ -1067,7 +1042,7 @@ static void its_cpu_init_collection(void) spin_unlock(&its_lock); } -static struct its_device *its_find_device(struct its_node *its, u32 dev_id) +struct its_device *its_find_device(struct its_node *its, u32 dev_id) { struct its_device *its_dev = NULL, *tmp; unsigned long flags; @@ -1086,7 +1061,7 @@ static struct its_device *its_find_device(struct its_node *its, u32 dev_id) return its_dev; } -static struct its_device *its_create_device(struct its_node *its, u32 dev_id, +struct its_device *its_create_device(struct its_node *its, u32 dev_id, int nvecs) { struct its_device *dev; diff --git a/include/linux/irq.h b/include/linux/irq.h index 62c6901..9a745a8 100644 --- a/include/linux/irq.h +++ b/include/linux/irq.h @@ -365,6 +365,7 @@ struct irq_chip { void (*irq_compose_msi_msg)(struct irq_data *data, struct msi_msg *msg); void (*irq_write_msi_msg)(struct irq_data *data, struct msi_msg *msg); + void (*irq_write_mbi_msg)(struct irq_data *irq_data, struct msi_msg *msg); int (*irq_get_irqchip_state)(struct irq_data *data, enum irqchip_irq_state which, bool *state); int (*irq_set_irqchip_state)(struct irq_data *data, enum irqchip_irq_state which, bool state); diff --git a/include/linux/irqchip/arm-gic-its.h b/include/linux/irqchip/arm-gic-its.h new file mode 100755 index 0000000..5677114 --- /dev/null +++ b/include/linux/irqchip/arm-gic-its.h @@ -0,0 +1,68 @@ +/* + * Copyright (C) 2013, 2014 ARM Limited, All Rights Reserved. + * Author: Marc Zyngier <marc.zyngier@arm.com> + * + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License version 2 as + * published by the Free Software Foundation. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program. If not, see <http://www.gnu.org/licenses/>. + */ +#ifndef __LINUX_IRQCHIP_ARM_GIC_ITS_H +#define __LINUX_IRQCHIP_ARM_GIC_ITS_H + +#include <linux/irqchip/arm-gic-v3.h> + +struct its_device { + struct list_head entry; + struct its_node *its; + struct its_collection *collection; + void *itt; + unsigned long *lpi_map; + irq_hw_number_t lpi_base; + int nr_lpis; + u32 nr_ites; + u32 device_id; +}; +/* + * Collection structure - just an ID, and a redistributor address to + * ping. We use one per CPU as a bag of interrupts assigned to this + * CPU. + */ +struct its_collection { + u64 target_address; + u16 col_id; +}; + +/* + * The ITS structure - contains most of the infrastructure, with the + * msi_controller, the command queue, the collections, and the list of + * devices writing to it. + */ +struct its_node { + raw_spinlock_t lock; + struct list_head entry; + struct msi_controller msi_chip; + struct irq_domain *domain; + void __iomem *base; + unsigned long phys_base; + struct its_cmd_block *cmd_base; + struct its_cmd_block *cmd_write; + void *tables[GITS_BASER_NR_REGS]; + struct its_collection *collections; + struct list_head its_device_list; + u64 flags; + u32 ite_size; +}; + +struct its_device *its_find_device(struct its_node *its, u32 dev_id); +struct its_device *its_create_device(struct its_node *its, u32 dev_id, + int nvecs); +#endif diff --git a/kernel/irq/chip.c b/kernel/irq/chip.c index eb9a4ea..3020efc 100644 --- a/kernel/irq/chip.c +++ b/kernel/irq/chip.c @@ -882,7 +882,8 @@ void irq_cpu_offline(void) void irq_chip_ack_parent(struct irq_data *data) { data = data->parent_data; - data->chip->irq_ack(data); + if (data->chip->irq_ack) + data->chip->irq_ack(data); } /** @@ -892,7 +893,8 @@ void irq_chip_ack_parent(struct irq_data *data) void irq_chip_mask_parent(struct irq_data *data) { data = data->parent_data; - data->chip->irq_mask(data); + if (data->chip->irq_mask) + data->chip->irq_mask(data); } /** @@ -902,7 +904,8 @@ void irq_chip_mask_parent(struct irq_data *data) void irq_chip_unmask_parent(struct irq_data *data) { data = data->parent_data; - data->chip->irq_unmask(data); + if (data->chip->irq_unmask) + data->chip->irq_unmask(data); } /** @@ -912,7 +915,8 @@ void irq_chip_unmask_parent(struct irq_data *data) void irq_chip_eoi_parent(struct irq_data *data) { data = data->parent_data; - data->chip->irq_eoi(data); + if (data->chip->irq_eoi) + data->chip->irq_eoi(data); } /**
This patch is applied to support the mbigen interrupt. Change log: --For irq_mbigen.c using,move some struct and function definition to a new head file arm-gic-its.h --Add a irq_write_mbi_msg member for mbi interrupt using --For mbi interrupt, the event id depends on the Hardware pin number on mbigen, so, the its_get_event_id is changed to calclulate the event id of mbi interrupt --For mbigen, the device id information need to be carried with msg. So, its_irq_compose_msi_msg is changed. --its_mask_msi_irq and its_unmaks_msi_irq are modifid for mbi interrupt using. --before the irq_ack callback, check the irq_ack first(chip.c) Signed-off-by: Ma Jun <majun258@huawei.com> --- drivers/irqchip/irq-gic-v3-its.c | 71 +++++++++++----------------------- include/linux/irq.h | 1 + include/linux/irqchip/arm-gic-its.h | 68 +++++++++++++++++++++++++++++++++ kernel/irq/chip.c | 12 ++++-- 4 files changed, 100 insertions(+), 52 deletions(-) mode change 100644 => 100755 drivers/irqchip/irq-gic-v3-its.c create mode 100755 include/linux/irqchip/arm-gic-its.h