Re: [PATCH 3/4]: Change arm-gic-its to support the Mbigen interrupt
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. -- Jazz is not dead. It just smells funny... -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 3/4]: Change arm-gic-its to support the Mbigen interrupt
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. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 3/4]: Change arm-gic-its to support the Mbigen interrupt
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. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 3/4]: Change arm-gic-its to support the Mbigen interrupt
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. -- Jazz is not dead. It just smells funny... -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 3/4]: Change arm-gic-its to support the Mbigen interrupt
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 > --- > 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 > #include > #include > +#include > #include > #include > #include > @@ -30,7 +31,7 @@ > #include > #include > > -#include > +#include > > #include > #include > @@ -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_headentry; > - 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_headits_device_list; > - u64 flags; > - u32 ite_size; > -}; > > #define ITS_ITT_ALIGNSZ_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_headentry; > - 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
Re: [PATCH 3/4]: Change arm-gic-its to support the Mbigen interrupt
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 majun...@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_headentry; - 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_headits_device_list; - u64 flags; - u32 ite_size; -}; #define ITS_ITT_ALIGNSZ_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_headentry; - 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
[PATCH 3/4]: Change arm-gic-its to support the Mbigen interrupt
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 --- 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 #include #include +#include #include #include #include @@ -30,7 +31,7 @@ #include #include -#include +#include #include #include @@ -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_headentry; - 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_headits_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_headentry; - 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 =
[PATCH 3/4]: Change arm-gic-its to support the Mbigen interrupt
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 majun...@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_headentry; - 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_headits_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_headentry; - 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