[v5,1/3] initialize each mbigen device node as a interrupt controller.

Commit Message

majun (F) Sept. 30, 2015, 9:39 a.m. UTC

From: Ma Jun <[e-mail]>

Mbigen means Message Based Interrupt Generator(MBIGEN).

Its a kind of interrupt controller that collects

the interrupts from external devices and generate msi interrupt.

Mbigen is applied to reduce the number of wire connected interrupts.

As the peripherals increasing, the interrupts lines needed is 
increasing much, especially on the Arm64 server soc.

Therefore, the interrupt pin in gic is not enough to cover so
many peripherals.

Mbigen is designed to fix this problem.

Mbigen chip locates in ITS or outside of ITS.

Mbigen chip hardware structure shows as below:

		mbigen chip
|---------------------|-------------------|
mgn_node0	  mgn_node1		mgn_node2
 |		 |-------|		|-------|------|        
dev1		dev1    dev2		dev1   dev3   dev4

Each mbigen chip contains several mbigen nodes.

External devices can connect to mbigen node through wire connecting way.

Because a mbigen node only can support 128 interrupt maximum, depends
on the interrupt lines number of devices, a device can connects to one
more mbigen nodes.

Also, several different devices can connect to a same mbigen node.

When devices triggered interrupt,mbigen chip detects and collects 
the interrupts and generates the MBI interrupts by writing the ITS
Translator register.

Signed-off-by: Ma Jun <[e-mail]>
---
 drivers/irqchip/irq-mbigen.c |  346 ++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 346 insertions(+), 0 deletions(-)
 create mode 100644 drivers/irqchip/irq-mbigen.c

Comments

Thomas Gleixner Sept. 30, 2015, 9:37 p.m. UTC | #1

On Wed, 30 Sep 2015, MaJun wrote:

First of all.

[PATCH v5 1/3] initialize each mbigen device node as a interrupt controller

is not a proper subject line, but that's the least of your problems.

> +#include <linux/init.h>
> +#include <linux/interrupt.h>
> +#include <linux/io.h>
> +#include <linux/irqchip/chained_irq.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/msi.h>
> +#include <linux/of_address.h>
> +#include <linux/of_irq.h>
> +#include <linux/of_platform.h>
> +#include <linux/platform_device.h>
> +#include <linux/slab.h>
> +#include "irqchip.h"

Do you really need all these includes?

> +
> +/* Interrupt numbers per mbigen node supported */
> +#define IRQS_PER_MBIGEN_NODE	(128)

Why do you need all these numbers in parentheses? Because you have all
these horrible MACROS?

> +/* Pin0-pin15 total 16 irqs are reserved for each mbigen chip*/
> +#define RESERVED_IRQ_PER_MBIGEN_CHIP (16)
> +
> +#define MBIGEN_INDEX_SHIFT	(12)
> +
> +/*
> + * To calculate the register addr of interrupt, the private index value
> + * also should be included except the hardware pin offset value.
> + *
> + * hwirq[23:12]: index. private index value of interrupt.
> +		Start from 0 for a device.
> + * hwirq[11:0]: pin. hardware pin offset of this interrupt
> + */
> +#define	COMPOSE_MBIGEN_HWIRQ(index, pin)	\
> +		(((index) << MBIGEN_INDEX_SHIFT) | (pin))

Please make that an inline function if at all.

> +
> +/* get the interrupt pin offset from mbigen hwirq */
> +#define	GET_IRQ_PIN_OFFSET(hwirq)	((hwirq) & 0xfff)

Ditto

> +/* get the private index value from mbigen hwirq */
> +#define GET_IRQ_INDEX(hwirq)	(((hwirq) >> MBIGEN_INDEX_SHIFT) & 0xfff)

Ditto

> +
> +/*
> + * In mbigen vector register
> + * bit[21:12]: event id value
> + * bit[11:0]: device id
> + */
> +#define IRQ_EVENT_ID_SHIFT	(12)
> +#define IRQ_EVENT_ID_MASK	(0x3ff)
> +
> +/* register range of mbigen node  */
> +#define MBIGEN_NODE_OFFSET	0x1000
> +
> +/* offset of vector register in mbigen node */
> +#define REG_MBIGEN_VEC_OFFSET	0x200
> +
> +/* offset of clear register in mbigen node.
> + * This register is used to clear the status
> + * of interrupt.
> + */
> +#define REG_MBIGEN_CLEAR_OFFSET	0xa00
> +
> +/*
> + * get the base address of mbigen node
> + * nid: mbigen node number
> + */
> +#define MBIGEN_NODE_ADDR_BASE(nid)	((nid) * MBIGEN_NODE_OFFSET)

That really does not help the readability of the code. Your MACRO is
longer than the actual term.

> +/*

For proper kerneldoc, this wants to be '/**'

> + * struct mbigen_device--Holds the  information of devices connected

I doubt, that '--' is a proper separator. ' - ' Definitely is.

> + * to mbigen chip

Please run that through the kerneldoc machinery.

> + * @domain: irq domain of this mbigen device.
> + * @global_entry: node in a global mbigen device list.
> + * @node: represents the mbigen device node defined in device tree.
> + * @mgn_data: pointer to mbigen_irq_data
> + * @nr_irqs: the total interrupt lines of this device
> + * @base: mapped address of mbigen chip which this mbigen device connected.

Can you please make this a proper table

+ * @domain:		irq domain of this mbigen device.
+ * @global_entry:	node in a global mbigen device list.

....

> +*/
> +struct mbigen_device {
> +	struct irq_domain	*domain;
> +	struct list_head	global_entry;
> +	struct device_node	*node;
> +	struct mbigen_irq_data	*mgn_data;
> +	unsigned int		nr_irqs;
> +	void __iomem		*base;
> +};
> +
> +/*
> + * struct irq_priv_info--structure of irq corresponding information.
> + *
> + * @global_pin_offset: global pin offset of this irq.
> + * @index: private index value of interrupt.(start from 0 for a device)
> + * @nid: id of mbigen node this irq connected.
> + * @local_pin_offset: local pin offset of interrupt within mbigen node.
> + * @reg_offset: Interrupt corresponding register addr offset.
> + */
> +struct irq_priv_info {
> +	unsigned int	global_pin_offset;
> +	unsigned int	index;
> +	unsigned int	nid;
> +	unsigned int	local_pin_offset;
> +	unsigned int	reg_offset;
> +};
> +
> +/*
> + * struct mbigen_irq_data -- private data of each irq
> + *
> + * @info: structure of irq private information.
> + * @dev: mbigen device this irq belong to.
> + * @dev_irq: virq number of this interrupt.
> + * @msi_irq: Corresponding msi irq number of this interrupt.
> + */
> +struct mbigen_irq_data {
> +	struct irq_priv_info	info;
> +	struct mbigen_device	*dev;
> +	unsigned int	dev_irq;
> +	unsigned int	msi_irq;
> +};
> +
> +/*
> + * global mbigen device list including all of the mbigen
> + * devices in this system
> + */
> +static LIST_HEAD(mbigen_device_list);
> +static DEFINE_SPINLOCK(mbigen_device_lock);
> +
> +static inline int get_mbigen_vec_reg_addr(u32 nid, u32 offset)
> +{
> +	return MBIGEN_NODE_ADDR_BASE(nid) + REG_MBIGEN_VEC_OFFSET
> +	    + (offset * 4);
> +}
> +
> +static struct mbigen_irq_data *get_mbigen_irq_data(struct mbigen_device *mgn_dev,
> +						   struct irq_data *d)
> +{
> +	struct irq_priv_info *info;
> +	u32 index;
> +
> +	index = GET_IRQ_INDEX(d->hwirq);
> +	if (index < 0)
> +		return NULL;

And how does index ever become < 0?

> +
> +	info = &mgn_dev->mgn_data[index].info;
> +	info->index = index;
> +	info->global_pin_offset = GET_IRQ_PIN_OFFSET(d->hwirq);
> +	info->nid = info->global_pin_offset / IRQS_PER_MBIGEN_NODE;
> +
> +	info->local_pin_offset = (info->global_pin_offset % IRQS_PER_MBIGEN_NODE)
> +						- RESERVED_IRQ_PER_MBIGEN_CHIP;
> +
> +	info->reg_offset = get_mbigen_vec_reg_addr(info->nid, info->local_pin_offset);

So you fill in a structure with 5 fields and the only information
which is ever used is local_pin_offset.

What's the point of this exercise? 

> +
> +	return &mgn_dev->mgn_data[index];
> +}
> +
> +static int mbigen_set_affinity(struct irq_data *data,
> +				const struct cpumask *mask_val,
> +				bool force)
> +{
> +	struct mbigen_irq_data *mgn_irq_data = irq_data_get_irq_chip_data(data);
> +	struct irq_chip *chip = irq_get_chip(mgn_irq_data->msi_irq);

And that msi_irq information comes from where? Nothing in that code
initializes it.

> +	struct irq_data *parent_d = irq_get_irq_data(mgn_irq_data->msi_irq);

Also WHY are you going through a full lookup of the chip and the irq
data, if that is your parent irq? That's what the domain hierarchy is
for. If you now tell me, that msi_irq is not the same as data->irq,
i.e. the virq number, then you have a lot more things to explain.

irq_chip_set_affinity_parent() is the callback you want for your chip,
not some completely nonsensical hackery.

> +
> +	if (chip && chip->irq_set_affinity)

Why would chip ever be NULL? If your parent interrupt does not have a
chip assigned then your whole setup is hosed.

> +static void mbigen_eoi_irq(struct irq_data *data)
> +{
> +
> +	struct mbigen_irq_data *mgn_irq_data = irq_data_get_irq_chip_data(data);
> +	struct mbigen_device *mgn_dev = mgn_irq_data->dev;

So the only reason for accessing yet another data structure is to get
the base address of that mbi device. You seem to have a strong
interest in making the cache foot print of your code as big as
possible. 

> +	struct irq_chip *chip = irq_get_chip(mgn_irq_data->msi_irq);
> +	struct irq_data *parent_d = irq_get_irq_data(mgn_irq_data->msi_irq);
> +	u32 pin_offset, ofst, mask;
> +
> +	pin_offset = mgn_irq_data->info.local_pin_offset;
> +
> +	ofst = pin_offset / 32 * 4;
> +	mask = 1 << (pin_offset % 32);
> +
> +	writel_relaxed(mask, mgn_dev->base + ofst
> +			+ REG_MBIGEN_CLEAR_OFFSET);
> +
> +	if (chip && chip->irq_eoi)
> +		chip->irq_eoi(parent_d);

So again. Why would chip be NULL and why would chip NOT have an EOI
callback?

> +static int mbigen_domain_xlate(struct irq_domain *d,
> +			       struct device_node *controller,
> +			       const u32 *intspec, unsigned int intsize,
> +			       unsigned long *out_hwirq,
> +			       unsigned int *out_type)
> +{
> +
> +	if (d->of_node != controller)
> +		return -EINVAL;
> +
> +	if (intsize < 2)
> +		return -EINVAL;
> +
> +	/* Compose the hwirq local to mbigen domain
> +	 * intspec[0]: interrut pin offset
> +	 * intspec[1]: index(start from 0)
> +	 */
> +	*out_hwirq = COMPOSE_MBIGEN_HWIRQ(intspec[1], intspec[0]);

So here you use that convoluted MACRO. Why can't you open code it so
we don't have to go up to the top of the file to see what you are
composing?

We use macros and inlines for things which are used over and over, but
not for code obfuscation.

> +static int mbigen_domain_map(struct irq_domain *d, unsigned int irq,
> +				irq_hw_number_t hw)
> +{
> +	struct mbigen_device *mgn_dev = d->host_data;
> +	struct mbigen_irq_data *mgn_irq_data;
> +	struct irq_data *data = irq_get_irq_data(irq);
> +
> +	mgn_irq_data = get_mbigen_irq_data(mgn_dev, data);
> +	if (!mgn_irq_data)
> +		return -EINVAL;

Ah. Here is that useless function actually called and of course the
return value which can never happen checked once more.

> +	mgn_irq_data->dev_irq = irq;

Oh, yet another place where you store the irq number. Darn, it's
already in irq_data. Your data representation is a complete mess.

All you ever need from this is local_pin_offset and the base address
for that calculation in the eoi callback.

> +	pin_offset = mgn_irq_data->info.local_pin_offset;
> +
> +	ofst = pin_offset / 32 * 4;
> +	mask = 1 << (pin_offset % 32);
> +
> +	writel_relaxed(mask, mgn_dev->base + ofst
> +			+ REG_MBIGEN_CLEAR_OFFSET);

Now if you think about it, then you might figure out, that you can
store that information in a way which does not require that math at
all and you can avoid having all these pointless data structures for
it. Hint: Each hierarchy level has it's own irq_data representation
and that is sufficient to store everything.

> +	irq_set_chip_data(irq, mgn_irq_data);
> +	irq_set_chip_and_handler(irq, &mbigen_irq_chip, handle_fasteoi_irq);
> +
> +	set_irq_flags(irq, IRQF_VALID);

And how does that compile against Linus kernel? Not at all.

> +
> +	/* add this mbigen device into a global list*/
> +	spin_lock(&mbigen_device_lock);
> +	list_add(&mgn_dev->global_entry, &mbigen_device_list);
> +	spin_unlock(&mbigen_device_lock);

And that global list is used whatfor? I can't see anything which makes
use of it.

That's a complete disaster and I'm not even thinking about looking at
the next patch in this series.

Can you please explain in a simple ASCII picture how your irq chip
hierarchy looks like and what kind of data you need for each hierarchy
level?

Thanks,

	tglx

majun (F) Oct. 4, 2015, 7:22 a.m. UTC | #2

Hi Thomas:

? 2015/10/1 5:37, Thomas Gleixner ??:
> On Wed, 30 Sep 2015, MaJun wrote:
> 
> First of all.
> 
> [PATCH v5 1/3] initialize each mbigen device node as a interrupt controller
> 
> is not a proper subject line, but that's the least of your problems.
> 
>> +#include <linux/init.h>
>> +#include <linux/interrupt.h>
>> +#include <linux/io.h>
>> +#include <linux/irqchip/chained_irq.h>
>> +#include <linux/kernel.h>
>> +#include <linux/module.h>
>> +#include <linux/msi.h>
>> +#include <linux/of_address.h>
>> +#include <linux/of_irq.h>
>> +#include <linux/of_platform.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/slab.h>
>> +#include "irqchip.h"
> 
> Do you really need all these includes?

Ok, I will remove usless includes.
> 
>> +
>> +/* Interrupt numbers per mbigen node supported */
>> +#define IRQS_PER_MBIGEN_NODE	(128)
> 
[...]
>> +
>> +	info = &mgn_dev->mgn_data[index].info;
>> +	info->index = index;
>> +	info->global_pin_offset = GET_IRQ_PIN_OFFSET(d->hwirq);
>> +	info->nid = info->global_pin_offset / IRQS_PER_MBIGEN_NODE;
>> +
>> +	info->local_pin_offset = (info->global_pin_offset % IRQS_PER_MBIGEN_NODE)
>> +						- RESERVED_IRQ_PER_MBIGEN_CHIP;
>> +
>> +	info->reg_offset = get_mbigen_vec_reg_addr(info->nid, info->local_pin_offset);
> 
> So you fill in a structure with 5 fields and the only information
> which is ever used is local_pin_offset.
> 
> What's the point of this exercise? 

Besides local_pin_offset , nid, and reg_offset are also useful information which will be used
in next patch.

For each mbigen chip, the register space of mbigen node between each other is discontinuous.
So, I need to find the mbigen node number(nid) and pin offset within this mbigen node
(local_pin_offset). Based on them, I can get the corresponding register address.

> 
>> +
>> +	return &mgn_dev->mgn_data[index];
>> +}
>> +
>> +static int mbigen_set_affinity(struct irq_data *data,
>> +				const struct cpumask *mask_val,
>> +				bool force)
>> +{
>> +	struct mbigen_irq_data *mgn_irq_data = irq_data_get_irq_chip_data(data);
>> +	struct irq_chip *chip = irq_get_chip(mgn_irq_data->msi_irq);
> 
> And that msi_irq information comes from where? Nothing in that code
> initializes it.

msi_irq is is initialized in next patch.

>> +	struct irq_data *parent_d = irq_get_irq_data(mgn_irq_data->msi_irq);
> 
> Also WHY are you going through a full lookup of the chip and the irq
> data, if that is your parent irq? That's what the domain hierarchy is
> for. If you now tell me, that msi_irq is not the same as data->irq,
> i.e. the virq number, then you have a lot more things to explain.

Yes, they have different virq number. My explanation about this
problem is list at last.


> irq_chip_set_affinity_parent() is the callback you want for your chip,
> not some completely nonsensical hackery.

I think this is used for hierarchy structure. My interrupt controller is not
hierarchy structrue.

> 
>> +
>> +	if (chip && chip->irq_set_affinity)
> 
> Why would chip ever be NULL? If your parent interrupt does not have a
> chip assigned then your whole setup is hosed.
> 
>> +static void mbigen_eoi_irq(struct irq_data *data)
>> +{
>> +
>> +	struct mbigen_irq_data *mgn_irq_data = irq_data_get_irq_chip_data(data);
>> +	struct mbigen_device *mgn_dev = mgn_irq_data->dev;
> 
> So the only reason for accessing yet another data structure is to get
> the base address of that mbi device. You seem to have a strong
> interest in making the cache foot print of your code as big as
> possible. 
> 
>> +	struct irq_chip *chip = irq_get_chip(mgn_irq_data->msi_irq);
>> +	struct irq_data *parent_d = irq_get_irq_data(mgn_irq_data->msi_irq);
>> +	u32 pin_offset, ofst, mask;
>> +
>> +	pin_offset = mgn_irq_data->info.local_pin_offset;
>> +
>> +	ofst = pin_offset / 32 * 4;
>> +	mask = 1 << (pin_offset % 32);
>> +
>> +	writel_relaxed(mask, mgn_dev->base + ofst
>> +			+ REG_MBIGEN_CLEAR_OFFSET);
>> +
>> +	if (chip && chip->irq_eoi)
>> +		chip->irq_eoi(parent_d);
> 
> So again. Why would chip be NULL and why would chip NOT have an EOI
> callback?
> 
>> +static int mbigen_domain_xlate(struct irq_domain *d,
>> +			       struct device_node *controller,
>> +			       const u32 *intspec, unsigned int intsize,
>> +			       unsigned long *out_hwirq,
>> +			       unsigned int *out_type)
>> +{
>> +
>> +	if (d->of_node != controller)
>> +		return -EINVAL;
>> +
>> +	if (intsize < 2)
>> +		return -EINVAL;
>> +
>> +	/* Compose the hwirq local to mbigen domain
>> +	 * intspec[0]: interrut pin offset
>> +	 * intspec[1]: index(start from 0)
>> +	 */
>> +	*out_hwirq = COMPOSE_MBIGEN_HWIRQ(intspec[1], intspec[0]);
> 
> So here you use that convoluted MACRO. Why can't you open code it so
> we don't have to go up to the top of the file to see what you are
> composing?
> 
> We use macros and inlines for things which are used over and over, but
> not for code obfuscation.
> 
>> +static int mbigen_domain_map(struct irq_domain *d, unsigned int irq,
>> +				irq_hw_number_t hw)
>> +{
>> +	struct mbigen_device *mgn_dev = d->host_data;
>> +	struct mbigen_irq_data *mgn_irq_data;
>> +	struct irq_data *data = irq_get_irq_data(irq);
>> +
>> +	mgn_irq_data = get_mbigen_irq_data(mgn_dev, data);
>> +	if (!mgn_irq_data)
>> +		return -EINVAL;
> 
> Ah. Here is that useless function actually called and of course the
> return value which can never happen checked once more.
> 
>> +	mgn_irq_data->dev_irq = irq;
> 
> Oh, yet another place where you store the irq number. Darn, it's
> already in irq_data. Your data representation is a complete mess.
> 
> All you ever need from this is local_pin_offset and the base address
> for that calculation in the eoi callback.

dev_irq is stored for easily using in next patch when interrupt happened.

> 
>> +	pin_offset = mgn_irq_data->info.local_pin_offset;
>> +
>> +	ofst = pin_offset / 32 * 4;
>> +	mask = 1 << (pin_offset % 32);
>> +
>> +	writel_relaxed(mask, mgn_dev->base + ofst
>> +			+ REG_MBIGEN_CLEAR_OFFSET);
> 
> Now if you think about it, then you might figure out, that you can
> store that information in a way which does not require that math at
> all and you can avoid having all these pointless data structures for
> it. Hint: Each hierarchy level has it's own irq_data representation
> and that is sufficient to store everything.
> 
>> +	irq_set_chip_data(irq, mgn_irq_data);
>> +	irq_set_chip_and_handler(irq, &mbigen_irq_chip, handle_fasteoi_irq);
>> +
>> +	set_irq_flags(irq, IRQF_VALID);
> 
> And how does that compile against Linus kernel? Not at all.
> 
>> +
>> +	/* add this mbigen device into a global list*/
>> +	spin_lock(&mbigen_device_lock);
>> +	list_add(&mgn_dev->global_entry, &mbigen_device_list);
>> +	spin_unlock(&mbigen_device_lock);
> 
> And that global list is used whatfor? I can't see anything which makes
> use of it.

This global list is used to find out mbigen device when initializing the mbigen
device as a platform device in next patch.

Because there are several mbigen chips in this system, and each mbigen chip also
contains several mbgien devices.

I need a list contains all of the mbigen devices which connect to these mbigen
chips.
Then, during mbigen chip initializing, we can use this list to find out mbigen devices
and pass mbigen_device data structure.

> 
> That's a complete disaster and I'm not even thinking about looking at
> the next patch in this series.
> 
> Can you please explain in a simple ASCII picture how your irq chip
> hierarchy looks like and what kind of data you need for each hierarchy
> level?

Mbigen chip hardware structure shows as below:

		mbigen chip
|---------------------|-------------------|
mgn_node0	  mgn_node1		mgn_node2
 |		 |-------|		|-------|------|
dev1		dev1    dev2		dev1   dev3   dev4



Irq chip hierarchy stucture:
		
	ITS
	 |
	ITS-pMSI
	 | (virq1)
|--------| -----------------|
mbigen-device1		mbigen-device2
 | (virq2)		 | (virq2)
devices(uart)		device(gmac)


I named virq1 as msi_irq , virq2 as dev-irq and ,virq1 != virq2.

Each virq2 has a corresponding virq1.

Mbigen-device is a special hardware.
On the one hand, it's a platform device for ITS. We need to
allocate the msi-irqs for it.(handled in patch 2/3)

On the other hand, it's a interrupt controller for the devices connected to it.(handled in current patch).

To bind these two different irqs, I made a data sutruce named mbigen_irq_data
which contains some information  of this irq, including private index, pin_offset, nid,
and local_pin_offset.

All these information can help us to find the corresponding reg addr and msi_irq quickly.


Thanks!
Ma Jun

Thomas Gleixner Oct. 9, 2015, 1:47 p.m. UTC | #3

On Sun, 4 Oct 2015, majun (F) wrote:
> >> +	info->reg_offset = get_mbigen_vec_reg_addr(info->nid, info->local_pin_offset);
> > 
> > So you fill in a structure with 5 fields and the only information
> > which is ever used is local_pin_offset.
> > 
> > What's the point of this exercise? 
> 
> Besides local_pin_offset , nid, and reg_offset are also useful
> information which will be used in next patch.

This is horrible to review, really.

Split your patches into smaller pieces then. Add the core
functionality and then introduce the other things when you actually
use them.

> > And that msi_irq information comes from where? Nothing in that code
> > initializes it.
> 
> msi_irq is is initialized in next patch.

See above.
 
> > All you ever need from this is local_pin_offset and the base address
> > for that calculation in the eoi callback.
> 
> dev_irq is stored for easily using in next patch when interrupt happened.

Ditto.
 
> >> +
> >> +	/* add this mbigen device into a global list*/
> >> +	spin_lock(&mbigen_device_lock);
> >> +	list_add(&mgn_dev->global_entry, &mbigen_device_list);
> >> +	spin_unlock(&mbigen_device_lock);
> > 
> > And that global list is used whatfor? I can't see anything which makes
> > use of it.
> 
> This global list is used to find out mbigen device when initializing the mbigen
> device as a platform device in next patch.

Sorry, this is unreviewable.
 
> Because there are several mbigen chips in this system, and each mbigen chip also
> contains several mbgien devices.
> 
> I need a list contains all of the mbigen devices which connect to these mbigen
> chips.
> Then, during mbigen chip initializing, we can use this list to find out mbigen devices
> and pass mbigen_device data structure.
> 
> > 
> > That's a complete disaster and I'm not even thinking about looking at
> > the next patch in this series.
> > 
> > Can you please explain in a simple ASCII picture how your irq chip
> > hierarchy looks like and what kind of data you need for each hierarchy
> > level?
> 
> Mbigen chip hardware structure shows as below:
> 
> 		mbigen chip
> |---------------------|-------------------|
> mgn_node0	  mgn_node1		mgn_node2
>  |		 |-------|		|-------|------|
> dev1		dev1    dev2		dev1   dev3   dev4
> 
> 
> 
> Irq chip hierarchy stucture:
> 		
> 	ITS
> 	 |
> 	ITS-pMSI
> 	 | (virq1)
> |--------| -----------------|
> mbigen-device1		mbigen-device2
>  | (virq2)		 | (virq2)
> devices(uart)		device(gmac)

That picture is wrong as it suggests that uart and gmac share the same
virq.
 
> I named virq1 as msi_irq , virq2 as dev-irq and ,virq1 != virq2.
> 
> Each virq2 has a corresponding virq1.

Whatfor?
 
> Mbigen-device is a special hardware.

Everything is special hardware.

> On the one hand, it's a platform device for ITS. We need to
> allocate the msi-irqs for it.(handled in patch 2/3)
> 
> On the other hand, it's a interrupt controller for the devices
> connected to it.(handled in current patch).
>
> To bind these two different irqs, I made a data sutruce named
> mbigen_irq_data which contains some information of this irq,
> including private index, pin_offset, nid, and local_pin_offset.
>
> All these information can help us to find the corresponding reg addr
> and msi_irq quickly.

This is completely wrong. Why would you need two linux virq numbers
for one interrupt?

This needs to be expressed in one hierarchy. mbigen is just a
translator between wired interrupts and MSI, nothing else.

So the hierarchy is:

  mbigen -> ITS-MSI -> ITS -> GIC

No need for extra levels of indirection. Your mbigen irqchip callbacks
are simply doing:

    parent->callback(parent_data);

and you get that for free when using the hierarchy. No need for that
chained interrupt handler either.

Thanks,

	tglx

majun (F) Oct. 10, 2015, 9:01 a.m. UTC | #4

? 2015/10/9 21:47, Thomas Gleixner ??:
> On Sun, 4 Oct 2015, majun (F) wrote:
>>>> +	info->reg_offset = get_mbigen_vec_reg_addr(info->nid, info->local_pin_offset);
>>>
>>> So you fill in a structure with 5 fields and the only information
[...]
>> On the other hand, it's a interrupt controller for the devices
>> connected to it.(handled in current patch).
>>
>> To bind these two different irqs, I made a data sutruce named
>> mbigen_irq_data which contains some information of this irq,
>> including private index, pin_offset, nid, and local_pin_offset.
>>
>> All these information can help us to find the corresponding reg addr
>> and msi_irq quickly.
> 
> This is completely wrong. Why would you need two linux virq numbers
> for one interrupt?
> 
> This needs to be expressed in one hierarchy. mbigen is just a
> translator between wired interrupts and MSI, nothing else.
> 
> So the hierarchy is:
> 
>   mbigen -> ITS-MSI -> ITS -> GIC

I think maybe you mean: mbigen -> ITS-pMSI -> ITS- > GIC

But there is a problem If i make the structure like you said.

For example, my hardware structure likes below:

uart ------> mbigen --> ITS-pMSI --> ITS --> GIC
     virq1

virq1 means the virq number allocted by irq_of_parse_and_map() function
when system parse the uart dts node in initializing  stage.

To create a ITS device, I need to call msi_domain_alloc_irqs() function
in my mbigen alloc function.

In this function, a new virq number(named as virq2 ) which different from
virq1 is allocated.
So, this is a big problem.

If we want to use the hierarchy structure, I think

mbigen -> ITS -> GIC

maybe is a possible way .

The only problem is I need to do some change in ITS driver.

I mean move its_create_device() and its_find_device() into
 its_irq_domain_alloc()

But this solution is similar to my v3 patch.

Thanks!
Ma Jun

















> 
> No need for extra levels of indirection. Your mbigen irqchip callbacks
> are simply doing:
> 
>     parent->callback(parent_data);
> 
> and you get that for free when using the hierarchy. No need for that
> chained interrupt handler either.
>

Marc Zyngier Oct. 10, 2015, 10:09 a.m. UTC | #5

On Sat, 10 Oct 2015 17:01:32 +0800
"majun (F)" <[e-mail]> wrote:

> 
> 
> ? 2015/10/9 21:47, Thomas Gleixner ??:
> > On Sun, 4 Oct 2015, majun (F) wrote:
> >>>> +	info->reg_offset = get_mbigen_vec_reg_addr(info->nid, info->local_pin_offset);
> >>>
> >>> So you fill in a structure with 5 fields and the only information
> [...]
> >> On the other hand, it's a interrupt controller for the devices
> >> connected to it.(handled in current patch).
> >>
> >> To bind these two different irqs, I made a data sutruce named
> >> mbigen_irq_data which contains some information of this irq,
> >> including private index, pin_offset, nid, and local_pin_offset.
> >>
> >> All these information can help us to find the corresponding reg addr
> >> and msi_irq quickly.
> > 
> > This is completely wrong. Why would you need two linux virq numbers
> > for one interrupt?
> > 
> > This needs to be expressed in one hierarchy. mbigen is just a
> > translator between wired interrupts and MSI, nothing else.
> > 
> > So the hierarchy is:
> > 
> >   mbigen -> ITS-MSI -> ITS -> GIC
> 
> I think maybe you mean: mbigen -> ITS-pMSI -> ITS- > GIC
> 
> But there is a problem If i make the structure like you said.
> 
> For example, my hardware structure likes below:
> 
> uart ------> mbigen --> ITS-pMSI --> ITS --> GIC
>      virq1
> 
> virq1 means the virq number allocted by irq_of_parse_and_map() function
> when system parse the uart dts node in initializing  stage.
> 
> To create a ITS device, I need to call msi_domain_alloc_irqs() function
> in my mbigen alloc function.
> 
> In this function, a new virq number(named as virq2 ) which different from
> virq1 is allocated.
> So, this is a big problem.

I think I see what your problem is:
- The wired interrupt (uart -> mbigen) is allocated through DT (and
  must be available early, because of of_platform_populate),
- The MSI (mgigen -> ITS) is dynamic (and allocated much later,
  because the device model kicks in after irqchip init, and we cannot
  allocate MSIs without a device).

So we end-up with two virqs that need to be reconciled one way or
another. Is that an accurate description of your problem?

This is a silly situation, because there is no multiplexing at all
(there is exactly one wired interrupt per MSI). It is just that we
don't have the infrastructure to connect the two virtual interrupts.

Thomas, what would you think of extending the MSI layer a bit so that
instead of allocating the virqs dynamically, we could take an
optional array of virqs that would be used? It should be pretty easy to
hack this into msi_domain_alloc_irqs(), and modify the existing callers
(or have a separate entry point altogether). I'm still a bit fuzzy on
how we stitch the domains themselves (we need to set ->parent on
allocating the MSIs, which leaves it being a NULL pointer until then),
but I think there is a way. Something like:

int msi_domain_alloc_stitch_irqs(struct irq_domain *domain,
				 struct irq_domain *parent,
				 struct device *dev,
				 int *virqs, int nvecs);

Naming is crap, but you'll get the idea. Thoughts?

> If we want to use the hierarchy structure, I think
> 
> mbigen -> ITS -> GIC
> 
> maybe is a possible way .
> 
> The only problem is I need to do some change in ITS driver.
> 
> I mean move its_create_device() and its_find_device() into
>  its_irq_domain_alloc()
> 
> But this solution is similar to my v3 patch.

And that was really bad. Let's not go back there.

Thanks,

	M.

Thomas Gleixner Oct. 11, 2015, 9:54 a.m. UTC | #6

On Sat, 10 Oct 2015, Marc Zyngier wrote:
> On Sat, 10 Oct 2015 17:01:32 +0800
> "majun (F)" <[e-mail]> wrote:
> > But there is a problem If i make the structure like you said.
> > 
> > For example, my hardware structure likes below:
> > 
> > uart ------> mbigen --> ITS-pMSI --> ITS --> GIC
> >      virq1
> > 
> > virq1 means the virq number allocted by irq_of_parse_and_map() function
> > when system parse the uart dts node in initializing  stage.
> > 
> > To create a ITS device, I need to call msi_domain_alloc_irqs() function
> > in my mbigen alloc function.
> > 
> > In this function, a new virq number(named as virq2 ) which different from
> > virq1 is allocated.
> > So, this is a big problem.
> 
> I think I see what your problem is:
> - The wired interrupt (uart -> mbigen) is allocated through DT (and
>   must be available early, because of of_platform_populate),
> - The MSI (mgigen -> ITS) is dynamic (and allocated much later,
>   because the device model kicks in after irqchip init, and we cannot
>   allocate MSIs without a device).

Why do we need that wired interrupt at all? 

We can make mbigen the 'msi-parent' of the device and let the
msi_domain_ops::msi_prepare() callback figure out the actual wiring
through device->fwnode.

Thanks,

	tglx

Marc Zyngier Oct. 11, 2015, 11:03 a.m. UTC | #7

On Sun, 11 Oct 2015 11:54:49 +0200
Thomas Gleixner <[e-mail]> wrote:

> On Sat, 10 Oct 2015, Marc Zyngier wrote:
> > On Sat, 10 Oct 2015 17:01:32 +0800
> > "majun (F)" <[e-mail]> wrote:
> > > But there is a problem If i make the structure like you said.
> > > 
> > > For example, my hardware structure likes below:
> > > 
> > > uart ------> mbigen --> ITS-pMSI --> ITS --> GIC
> > >      virq1
> > > 
> > > virq1 means the virq number allocted by irq_of_parse_and_map() function
> > > when system parse the uart dts node in initializing  stage.
> > > 
> > > To create a ITS device, I need to call msi_domain_alloc_irqs() function
> > > in my mbigen alloc function.
> > > 
> > > In this function, a new virq number(named as virq2 ) which different from
> > > virq1 is allocated.
> > > So, this is a big problem.
> > 
> > I think I see what your problem is:
> > - The wired interrupt (uart -> mbigen) is allocated through DT (and
> >   must be available early, because of of_platform_populate),
> > - The MSI (mgigen -> ITS) is dynamic (and allocated much later,
> >   because the device model kicks in after irqchip init, and we cannot
> >   allocate MSIs without a device).
> 
> Why do we need that wired interrupt at all? 
> 
> We can make mbigen the 'msi-parent' of the device and let the
> msi_domain_ops::msi_prepare() callback figure out the actual wiring
> through device->fwnode.

That's because the device behind the mbigen can't do any MSI at all.
Think of a 8250 uart, for example.

If we make the mbigen the msi-parent of the uart, then we need to teach
the 8250 driver to request MSIs. It also means that the DT doesn't
represent the HW anymore (this wired interrupt actually exists).

Thanks,

	M.

Thomas Gleixner Oct. 11, 2015, 4:45 p.m. UTC | #8

On Sun, 11 Oct 2015, Marc Zyngier wrote:
> On Sun, 11 Oct 2015 11:54:49 +0200
> Thomas Gleixner <[e-mail]> wrote:
> 
> > On Sat, 10 Oct 2015, Marc Zyngier wrote:
> > > On Sat, 10 Oct 2015 17:01:32 +0800
> > > "majun (F)" <[e-mail]> wrote:
> > > > But there is a problem If i make the structure like you said.
> > > > 
> > > > For example, my hardware structure likes below:
> > > > 
> > > > uart ------> mbigen --> ITS-pMSI --> ITS --> GIC
> > > >      virq1
> > > > 
> > > > virq1 means the virq number allocted by irq_of_parse_and_map() function
> > > > when system parse the uart dts node in initializing  stage.
> > > > 
> > > > To create a ITS device, I need to call msi_domain_alloc_irqs() function
> > > > in my mbigen alloc function.
> > > > 
> > > > In this function, a new virq number(named as virq2 ) which different from
> > > > virq1 is allocated.
> > > > So, this is a big problem.
> > > 
> > > I think I see what your problem is:
> > > - The wired interrupt (uart -> mbigen) is allocated through DT (and
> > >   must be available early, because of of_platform_populate),
> > > - The MSI (mgigen -> ITS) is dynamic (and allocated much later,
> > >   because the device model kicks in after irqchip init, and we cannot
> > >   allocate MSIs without a device).
> > 
> > Why do we need that wired interrupt at all? 
> > 
> > We can make mbigen the 'msi-parent' of the device and let the
> > msi_domain_ops::msi_prepare() callback figure out the actual wiring
> > through device->fwnode.
> 
> That's because the device behind the mbigen can't do any MSI at all.
> Think of a 8250 uart, for example.
> 
> If we make the mbigen the msi-parent of the uart, then we need to teach
> the 8250 driver to request MSIs.

I really do not understand why of_platform_populate cares about
interrupt allocations. That's outright stupid. We should care about
that at device probe time, i.e. at the point where the driver is
registered and probed if there is matching platform data. If we do it
in of_platform_populate then we allocate interrupts even for devices
which do not have a driver, i.e. we just waste memory.

So we better teach a couple of drivers to handle that instead of
inventing horrible workarounds.

> It also means that the DT doesn't represent the HW anymore (this
> wired interrupt actually exists).

I think the abstraction here is wrong. If it would be correct, then
PCI-MSI would be wrong. The MSI part of PCI is a MSI producer, mbigen
is as well. Technically MSI is not integral part of the PCI device, it
just happens to have it's configuration registers in the PCI
configuration space of the device:

    [PCI-BUS]------[Interrupt mode selector]
    	      	|        |
    	      	|        |
		------[Legacy irq gate]<-----
		|        |                  |
		|        |                  |---[Device interrupt]
		|        |                  |
		------[MSI unit]<------------

So you have a 'wire' from the device to the MSI unit, but we do not
care about that 'wire'. All we care about are the MSI configuration
registers. We find them via the PCI device which gives us the address
of the PCI configuration space.

So now in the mbigen case this looks like this:

    [MSI-BUS] ----- [MBIGEN]<-------------------[Device interrupt]

Again, you have a 'wire' from the device to the MSI unit (MBIGEN) and
we do not care about that 'wire' either. What we care about is how we
find the MSI (mbigen) configuration registers for a particular
device. So we need a DT/ACPI entry which describes those configuration
registers and whatever supplementary information is required. That
will make the mbigen driver extremly simple.

Thanks,

	tglx

majun (F) Oct. 13, 2015, 6:32 a.m. UTC | #9

Hi Thomas:

? 2015/10/12 0:45, Thomas Gleixner ??:
> On Sun, 11 Oct 2015, Marc Zyngier wrote:
>> On Sun, 11 Oct 2015 11:54:49 +0200
>> Thomas Gleixner <[e-mail]> wrote:
>>
>>> On Sat, 10 Oct 2015, Marc Zyngier wrote:
>>>> On Sat, 10 Oct 2015 17:01:32 +0800
>>>> "majun (F)" <[e-mail]> wrote:
>>>>> But there is a problem If i make the structure like you said.
>>>>>
>>>>> For example, my hardware structure likes below:
>>>>>
>>>>> uart ------> mbigen --> ITS-pMSI --> ITS --> GIC
>>>>>      virq1
>>>>>
>>>>> virq1 means the virq number allocted by irq_of_parse_and_map() function
>>>>> when system parse the uart dts node in initializing  stage.
>>>>>
>>>>> To create a ITS device, I need to call msi_domain_alloc_irqs() function
>>>>> in my mbigen alloc function.
>>>>>
>>>>> In this function, a new virq number(named as virq2 ) which different from
>>>>> virq1 is allocated.
>>>>> So, this is a big problem.
>>>>
>>>> I think I see what your problem is:
>>>> - The wired interrupt (uart -> mbigen) is allocated through DT (and
>>>>   must be available early, because of of_platform_populate),
>>>> - The MSI (mgigen -> ITS) is dynamic (and allocated much later,
>>>>   because the device model kicks in after irqchip init, and we cannot
>>>>   allocate MSIs without a device).
>>>
>>> Why do we need that wired interrupt at all? 
>>>
>>> We can make mbigen the 'msi-parent' of the device and let the
>>> msi_domain_ops::msi_prepare() callback figure out the actual wiring
>>> through device->fwnode.
>>
>> That's because the device behind the mbigen can't do any MSI at all.
>> Think of a 8250 uart, for example.
>>
>> If we make the mbigen the msi-parent of the uart, then we need to teach
>> the 8250 driver to request MSIs.
> 
> I really do not understand why of_platform_populate cares about
> interrupt allocations. That's outright stupid. We should care about
> that at device probe time, i.e. at the point where the driver is
> registered and probed if there is matching platform data. If we do it
> in of_platform_populate then we allocate interrupts even for devices
> which do not have a driver, i.e. we just waste memory.
> 
> So we better teach a couple of drivers to handle that instead of
> inventing horrible workarounds.
> 
>> It also means that the DT doesn't represent the HW anymore (this
>> wired interrupt actually exists).
> 
> I think the abstraction here is wrong. If it would be correct, then
> PCI-MSI would be wrong. The MSI part of PCI is a MSI producer, mbigen
> is as well. Technically MSI is not integral part of the PCI device, it
> just happens to have it's configuration registers in the PCI
> configuration space of the device:
> 
>     [PCI-BUS]------[Interrupt mode selector]
>     	      	|        |
>     	      	|        |
> 		------[Legacy irq gate]<-----
> 		|        |                  |
> 		|        |                  |---[Device interrupt]
> 		|        |                  |
> 		------[MSI unit]<------------
> 
> So you have a 'wire' from the device to the MSI unit, but we do not
> care about that 'wire'. All we care about are the MSI configuration
> registers. We find them via the PCI device which gives us the address
> of the PCI configuration space.
> 
> So now in the mbigen case this looks like this:
> 
>     [MSI-BUS] ----- [MBIGEN]<-------------------[Device interrupt]
> 
> Again, you have a 'wire' from the device to the MSI unit (MBIGEN) and
> we do not care about that 'wire' either. What we care about is how we
> find the MSI (mbigen) configuration registers for a particular
> device. So we need a DT/ACPI entry which describes those configuration
> registers and whatever supplementary information is required. That
> will make the mbigen driver extremly simple.
> 

According to your suggestions, I tried to make the hardware structure likes below:

device(8250 uart) -> mbigne -> ITS-pMSI --> ITS --> GIC

And  8250 uart dts node is:

 8250_uart {
	compatible = "xxx";
	msi-parent = < &mbigen>;
	config_addr = <xxxxx> ; /* configuration register */
	interrupts = <x>;
	interrupt-parent = ?
}

My question is what's the interrupt-parent should be?

Thomas Gleixner Oct. 13, 2015, 6:55 a.m. UTC | #10

Majun,

On Tue, 13 Oct 2015, majun (F) wrote:
> ? 2015/10/12 0:45, Thomas Gleixner ??:
> > So now in the mbigen case this looks like this:
> > 
> >     [MSI-BUS] ----- [MBIGEN]<-------------------[Device interrupt]
> > 
> > Again, you have a 'wire' from the device to the MSI unit (MBIGEN) and
> > we do not care about that 'wire' either. What we care about is how we
> > find the MSI (mbigen) configuration registers for a particular
> > device. So we need a DT/ACPI entry which describes those configuration
> > registers and whatever supplementary information is required. That
> > will make the mbigen driver extremly simple.
> > 
> 
> According to your suggestions, I tried to make the hardware structure likes below:
> 
> device(8250 uart) -> mbigne -> ITS-pMSI --> ITS --> GIC

I'm not sure whether mbigen should be connected to ITS-pMSI (I assume
you mean ITS-PCI-MSI).

mbigen is a seperate MSI domain, so it should connect to ITS, but I
leave that to Marc.
 
> And  8250 uart dts node is:
> 
>  8250_uart {
> 	compatible = "xxx";
> 	msi-parent = < &mbigen>;
> 	config_addr = <xxxxx> ; /* configuration register */
> 	interrupts = <x>;
> 	interrupt-parent = ?
> }
> 
> My question is what's the interrupt-parent should be?

There is no interrupt parent for 8250_uart. Why would you want that?
I'm really not a DT expert, but I think you want something like this:

  8250_uart {
 	compatible = "xxx";
 	msi-parent = < &mbigen_node5>;
 	interrupt-map = <&mbigen5 0>;
  };

and then have

  mbigen_node5 {
  	...
	reg = <....>;
  };

So the other devices which are connected to mbigen_node5 have the same
msi-parent. But then again, please discuss that with Marc and the DT
wizards.

Thanks,

	tglx

majun (F) Oct. 14, 2015, 8:16 a.m. UTC | #11

Hi Thomas:

? 2015/10/13 14:55, Thomas Gleixner ??:
> Majun,
> 
> On Tue, 13 Oct 2015, majun (F) wrote:
>> ? 2015/10/12 0:45, Thomas Gleixner ??:
>>> So now in the mbigen case this looks like this:
>>>
>>>     [MSI-BUS] ----- [MBIGEN]<-------------------[Device interrupt]
>>>
>>> Again, you have a 'wire' from the device to the MSI unit (MBIGEN) and
>>> we do not care about that 'wire' either. What we care about is how we
>>> find the MSI (mbigen) configuration registers for a particular
>>> device. So we need a DT/ACPI entry which describes those configuration
>>> registers and whatever supplementary information is required. That
>>> will make the mbigen driver extremly simple.
>>>
>>
>> According to your suggestions, I tried to make the hardware structure likes below:
>>
>> device(8250 uart) -> mbigne -> ITS-pMSI --> ITS --> GIC
> 
> I'm not sure whether mbigen should be connected to ITS-pMSI (I assume
> you mean ITS-PCI-MSI).

ITS domain has two child domains. One is ITS-pMSI for Non-PCI devices,
the other one is ITS-MSI for PCI devices.

> 
> mbigen is a seperate MSI domain, so it should connect to ITS, but I
> leave that to Marc.

I also think mbigen should connected to ITS.

Now, the hierarchy structure is
MBIGEN -> ITS -> GIC.

This structure is really similar to the structure in my v3 patch except the
dts.

>  
>> And  8250 uart dts node is:
>>
>>  8250_uart {
>> 	compatible = "xxx";
>> 	msi-parent = < &mbigen>;
>> 	config_addr = <xxxxx> ; /* configuration register */
>> 	interrupts = <x>;
>> 	interrupt-parent = ?
>> }
>>
>> My question is what's the interrupt-parent should be?
> 
> There is no interrupt parent for 8250_uart. Why would you want that?
> I'm really not a DT expert, but I think you want something like this:
> 
>   8250_uart {
>  	compatible = "xxx";
>  	msi-parent = < &mbigen_node5>;
>  	interrupt-map = <&mbigen5 0>;
>   };
> 

Maybe you mean
interrupt-map = <&mbigen_node5 0>;

I have some questions about this

[1]: I noticed there is no interrupts property,
So, do you mean we don't need this property here ?

[2]: I am confused about interrupt-map.

This property is parsed in function of_irq_parse_raw().
In this fucntion

"
ipar = of_node_get(out_irq->np);
of_get_property(ipar, "interrupt-map", &imaplen);
"
When this function is called by of_irq_parse_one(),
the input para out_irq->np pointed to GIC or mbigen (depends on dts).

So, of_get_property() tried to get the interrupt-map from
GIC or mbgien node.

But there is no interrupt-map property in GIC or mbigen node.



> and then have
> 
>   mbigen_node5 {
>   	...
> 	reg = <....>;
>   };
> 
> So the other devices which are connected to mbigen_node5 have the same
> msi-parent. But then again, please discuss that with Marc and the DT
> wizards.
>

Thomas Gleixner Oct. 14, 2015, 8:20 a.m. UTC | #12

On Wed, 14 Oct 2015, majun (F) wrote:
> But there is no interrupt-map property in GIC or mbigen node.

Again: I'm not a DT expert. Please discuss that with Marc and the DT
wizards.

Thanks,

	tglx

majun (F) Oct. 14, 2015, 8:54 a.m. UTC | #13

? 2015/10/14 16:20, Thomas Gleixner ??:
> On Wed, 14 Oct 2015, majun (F) wrote:
>> But there is no interrupt-map property in GIC or mbigen node.
> 
> Again: I'm not a DT expert. Please discuss that with Marc and the DT
> wizards.
> 
> Thanks,
> 
> 	tglx
> 
> .

Ok.
Marc, what's your suggestion ? much appreciated

Thanks!
Ma Jun


>

Marc Zyngier Oct. 14, 2015, 8:55 a.m. UTC | #14

Hi Thomas,

Sorry it took me so long to come back to you on that one, I really
needed to wrap my head around it.

On 11/10/15 17:45, Thomas Gleixner wrote:
> On Sun, 11 Oct 2015, Marc Zyngier wrote:
>> On Sun, 11 Oct 2015 11:54:49 +0200
>> Thomas Gleixner <[e-mail]> wrote:
>>
>>> On Sat, 10 Oct 2015, Marc Zyngier wrote:
>>>> On Sat, 10 Oct 2015 17:01:32 +0800
>>>> "majun (F)" <[e-mail]> wrote:
>>>>> But there is a problem If i make the structure like you said.
>>>>>
>>>>> For example, my hardware structure likes below:
>>>>>
>>>>> uart ------> mbigen --> ITS-pMSI --> ITS --> GIC
>>>>>      virq1
>>>>>
>>>>> virq1 means the virq number allocted by irq_of_parse_and_map() function
>>>>> when system parse the uart dts node in initializing  stage.
>>>>>
>>>>> To create a ITS device, I need to call msi_domain_alloc_irqs() function
>>>>> in my mbigen alloc function.
>>>>>
>>>>> In this function, a new virq number(named as virq2 ) which different from
>>>>> virq1 is allocated.
>>>>> So, this is a big problem.
>>>>
>>>> I think I see what your problem is:
>>>> - The wired interrupt (uart -> mbigen) is allocated through DT (and
>>>>   must be available early, because of of_platform_populate),
>>>> - The MSI (mgigen -> ITS) is dynamic (and allocated much later,
>>>>   because the device model kicks in after irqchip init, and we cannot
>>>>   allocate MSIs without a device).
>>>
>>> Why do we need that wired interrupt at all? 
>>>
>>> We can make mbigen the 'msi-parent' of the device and let the
>>> msi_domain_ops::msi_prepare() callback figure out the actual wiring
>>> through device->fwnode.
>>
>> That's because the device behind the mbigen can't do any MSI at all.
>> Think of a 8250 uart, for example.
>>
>> If we make the mbigen the msi-parent of the uart, then we need to teach
>> the 8250 driver to request MSIs.
> 
> I really do not understand why of_platform_populate cares about
> interrupt allocations. That's outright stupid. We should care about
> that at device probe time, i.e. at the point where the driver is
> registered and probed if there is matching platform data. If we do it
> in of_platform_populate then we allocate interrupts even for devices
> which do not have a driver, i.e. we just waste memory.

This was introduce for a specific reason: being able to convert systems
from board files to DT without having to DT-ify drivers. Interrupt
numbers magically appear as part of the resource array, and everything
just works.

> 
> So we better teach a couple of drivers to handle that instead of
> inventing horrible workarounds.
> 
>> It also means that the DT doesn't represent the HW anymore (this
>> wired interrupt actually exists).
> 
> I think the abstraction here is wrong. If it would be correct, then
> PCI-MSI would be wrong. The MSI part of PCI is a MSI producer, mbigen
> is as well. Technically MSI is not integral part of the PCI device, it
> just happens to have it's configuration registers in the PCI
> configuration space of the device:

The main difference is that the MSI layer is actually specified in PCI.
Yes, the wire is conveniently hidden from us, but that's also because it
can be hidden: by construction, you have an MSI generator per device.

Mbigen breaks this rule: you can have an MSI generator that covers
multiple devices, or a single device that spans multiple generators.
Nothing that can't be overcome, but that makes things a bit ugly.

>     [PCI-BUS]------[Interrupt mode selector]
>     	      	|        |
>     	      	|        |
> 		------[Legacy irq gate]<-----
> 		|        |                  |
> 		|        |                  |---[Device interrupt]
> 		|        |                  |
> 		------[MSI unit]<------------
> 
> So you have a 'wire' from the device to the MSI unit, but we do not
> care about that 'wire'. All we care about are the MSI configuration
> registers. We find them via the PCI device which gives us the address
> of the PCI configuration space.
> 
> So now in the mbigen case this looks like this:
> 
>     [MSI-BUS] ----- [MBIGEN]<-------------------[Device interrupt]
> 
> Again, you have a 'wire' from the device to the MSI unit (MBIGEN) and
> we do not care about that 'wire' either. What we care about is how we
> find the MSI (mbigen) configuration registers for a particular
> device. So we need a DT/ACPI entry which describes those configuration
> registers and whatever supplementary information is required. That
> will make the mbigen driver extremly simple.

It makes mbigen simple indeed, but it also makes other parts more complex:

- mbigen needs to implement its own MSI layer (we cannot use the
platform one for that)

- it effectively becomes part of the ITS driver (it needs to pass the
device IDs down to the core ITS code instead of relying on the ITS
platform MSI code).

- this probably means introducing some new probing infrastructure so
that the device msi_domain field can be populated with the right domain
(I'm not sure the platform probing does the right thing in that case)

- drivers that aren't MSI aware need to be extended to talk to this new
MSI provider.

- drivers that already support MSI natively (like the ARM SMMUv3, see
http://www.spinics.net/lists/arm-kernel/msg452057.html) will have to be
hacked to also work with this new method.

To me, it feels like we're spreading the complexity across multiple
layers instead of keeping it localized. It also means that next time
some crazy HW dude comes up with a similar idea (and I have little doubt
this will happen sooner than later), we'll have to replicate the same
thing again (though we could put all that behind another abstraction layer).

I would have preferred a solution where the MSI domain is allowed to be
sandwiched between two non-MSI domains, and expose the top level
irqchip. This means fixing the following:

- Either find a way to prevent DT doing these early IRQ allocations
(this could be easily done by simply not registering the irqchip), or be
able to elegantly reuse them.

- Add an API allowing an MSI domain to be the parent of another domain.

Once we have this, we can use the platform MSI layer for the mbigen
without much complexity (well, not more that any other stacked irqchip,
the madness of the mbigen programming interface notwithstanding), and
drivers stay untouched. It would also give us a 'standard' way to deal
with the above HW dude. I'd be happy to prototype it.

Now, I'm not going to make that a religious affair. If you still think
I'm severely misguided, I'll stop arguing and we'll make it work
according to your plans.

You can now open fire! ;-)

	M.

Thomas Gleixner Oct. 14, 2015, 9:17 a.m. UTC | #15

Marc,

On Wed, 14 Oct 2015, Marc Zyngier wrote:
> To me, it feels like we're spreading the complexity across multiple
> layers instead of keeping it localized. It also means that next time
> some crazy HW dude comes up with a similar idea (and I have little doubt
> this will happen sooner than later), we'll have to replicate the same
> thing again (though we could put all that behind another abstraction layer).
> 
> I would have preferred a solution where the MSI domain is allowed to be
> sandwiched between two non-MSI domains, and expose the top level
> irqchip. This means fixing the following:
> 
> - Either find a way to prevent DT doing these early IRQ allocations
> (this could be easily done by simply not registering the irqchip), or be
> able to elegantly reuse them.

The reuse part makes me shudder. We really should not go there. It's a
blatant layering violation.

> - Add an API allowing an MSI domain to be the parent of another domain.
> 
> Once we have this, we can use the platform MSI layer for the mbigen
> without much complexity (well, not more that any other stacked irqchip,
> the madness of the mbigen programming interface notwithstanding), and
> drivers stay untouched. It would also give us a 'standard' way to deal
> with the above HW dude. I'd be happy to prototype it.

Ok, I have a better understanding of it now.

I have no objections to your approach as long as it provides us a
clean way to use a full hierarchy without weird interfaces to reuse
irq descriptors etc. If you can find a way which just follows the
proper hierarchy design, I'm certainly not in your way.

OTOH, the platform msi driver is not a huge amount of code and from my
understanding of the hardware it looks weird to have this intermediate
layer. Making mbigen a direct child of ITS feels just more natural to
me. I'm pretty sure that this can be done without the earlier proposed
horrible modifications to ITS. It just should fall into place.

It would be really great just to have shell implementations,
i.e. without the mbigen specific stuff - for both models so we can
compare and contrast the results. That means just the interfaces and
the hookup to the various layers.

Thanks,

	tglx

Marc Zyngier Oct. 14, 2015, 9:49 a.m. UTC | #16

On 14/10/15 10:17, Thomas Gleixner wrote:
> Marc,
> 
> On Wed, 14 Oct 2015, Marc Zyngier wrote:
>> To me, it feels like we're spreading the complexity across multiple
>> layers instead of keeping it localized. It also means that next time
>> some crazy HW dude comes up with a similar idea (and I have little doubt
>> this will happen sooner than later), we'll have to replicate the same
>> thing again (though we could put all that behind another abstraction layer).
>>
>> I would have preferred a solution where the MSI domain is allowed to be
>> sandwiched between two non-MSI domains, and expose the top level
>> irqchip. This means fixing the following:
>>
>> - Either find a way to prevent DT doing these early IRQ allocations
>> (this could be easily done by simply not registering the irqchip), or be
>> able to elegantly reuse them.
> 
> The reuse part makes me shudder. We really should not go there. It's a
> blatant layering violation.
> 
>> - Add an API allowing an MSI domain to be the parent of another domain.
>>
>> Once we have this, we can use the platform MSI layer for the mbigen
>> without much complexity (well, not more that any other stacked irqchip,
>> the madness of the mbigen programming interface notwithstanding), and
>> drivers stay untouched. It would also give us a 'standard' way to deal
>> with the above HW dude. I'd be happy to prototype it.
> 
> Ok, I have a better understanding of it now.
> 
> I have no objections to your approach as long as it provides us a
> clean way to use a full hierarchy without weird interfaces to reuse
> irq descriptors etc. If you can find a way which just follows the
> proper hierarchy design, I'm certainly not in your way.
> 
> OTOH, the platform msi driver is not a huge amount of code and from my
> understanding of the hardware it looks weird to have this intermediate
> layer. Making mbigen a direct child of ITS feels just more natural to
> me. I'm pretty sure that this can be done without the earlier proposed
> horrible modifications to ITS. It just should fall into place.
> 
> It would be really great just to have shell implementations,
> i.e. without the mbigen specific stuff - for both models so we can
> compare and contrast the results. That means just the interfaces and
> the hookup to the various layers.

OK, let's do that. I'll try to post something before Friday so that we
can really evaluate what the knock-on effect is.

Thanks,

	M.

Patch

diff --git a/drivers/irqchip/irq-mbigen.c b/drivers/irqchip/irq-mbigen.c
new file mode 100644
index 0000000..e05a0ed
--- /dev/null
+++ b/drivers/irqchip/irq-mbigen.c
@@ -0,0 +1,346 @@ 
+/*
+ * Copyright (C) 2015 Hisilicon Limited, All Rights Reserved.
+ * Author: Jun Ma <[e-mail]>
+ * Author: Yun Wu <[e-mail]>
+ *
+ * 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/>.
+ */
+
+#include <linux/init.h>
+#include <linux/interrupt.h>
+#include <linux/io.h>
+#include <linux/irqchip/chained_irq.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/msi.h>
+#include <linux/of_address.h>
+#include <linux/of_irq.h>
+#include <linux/of_platform.h>
+#include <linux/platform_device.h>
+#include <linux/slab.h>
+#include "irqchip.h"
+
+
+/* Interrupt numbers per mbigen node supported */
+#define IRQS_PER_MBIGEN_NODE	(128)
+
+/* Pin0-pin15 total 16 irqs are reserved for each mbigen chip*/
+#define RESERVED_IRQ_PER_MBIGEN_CHIP (16)
+
+#define MBIGEN_INDEX_SHIFT	(12)
+
+/*
+ * To calculate the register addr of interrupt, the private index value
+ * also should be included except the hardware pin offset value.
+ *
+ * hwirq[23:12]: index. private index value of interrupt.
+		Start from 0 for a device.
+ * hwirq[11:0]: pin. hardware pin offset of this interrupt
+ */
+#define	COMPOSE_MBIGEN_HWIRQ(index, pin)	\
+		(((index) << MBIGEN_INDEX_SHIFT) | (pin))
+
+/* get the interrupt pin offset from mbigen hwirq */
+#define	GET_IRQ_PIN_OFFSET(hwirq)	((hwirq) & 0xfff)
+/* get the private index value from mbigen hwirq */
+#define GET_IRQ_INDEX(hwirq)	(((hwirq) >> MBIGEN_INDEX_SHIFT) & 0xfff)
+
+/*
+ * In mbigen vector register
+ * bit[21:12]: event id value
+ * bit[11:0]: device id
+ */
+#define IRQ_EVENT_ID_SHIFT	(12)
+#define IRQ_EVENT_ID_MASK	(0x3ff)
+
+/* register range of mbigen node  */
+#define MBIGEN_NODE_OFFSET	0x1000
+
+/* offset of vector register in mbigen node */
+#define REG_MBIGEN_VEC_OFFSET	0x200
+
+/* offset of clear register in mbigen node.
+ * This register is used to clear the status
+ * of interrupt.
+ */
+#define REG_MBIGEN_CLEAR_OFFSET	0xa00
+
+/*
+ * get the base address of mbigen node
+ * nid: mbigen node number
+ */
+#define MBIGEN_NODE_ADDR_BASE(nid)	((nid) * MBIGEN_NODE_OFFSET)
+
+/*
+ * struct mbigen_device--Holds the  information of devices connected
+ * to mbigen chip
+ * @domain: irq domain of this mbigen device.
+ * @global_entry: node in a global mbigen device list.
+ * @node: represents the mbigen device node defined in device tree.
+ * @mgn_data: pointer to mbigen_irq_data
+ * @nr_irqs: the total interrupt lines of this device
+ * @base: mapped address of mbigen chip which this mbigen device connected.
+*/
+struct mbigen_device {
+	struct irq_domain	*domain;
+	struct list_head	global_entry;
+	struct device_node	*node;
+	struct mbigen_irq_data	*mgn_data;
+	unsigned int		nr_irqs;
+	void __iomem		*base;
+};
+
+/*
+ * struct irq_priv_info--structure of irq corresponding information.
+ *
+ * @global_pin_offset: global pin offset of this irq.
+ * @index: private index value of interrupt.(start from 0 for a device)
+ * @nid: id of mbigen node this irq connected.
+ * @local_pin_offset: local pin offset of interrupt within mbigen node.
+ * @reg_offset: Interrupt corresponding register addr offset.
+ */
+struct irq_priv_info {
+	unsigned int	global_pin_offset;
+	unsigned int	index;
+	unsigned int	nid;
+	unsigned int	local_pin_offset;
+	unsigned int	reg_offset;
+};
+
+/*
+ * struct mbigen_irq_data -- private data of each irq
+ *
+ * @info: structure of irq private information.
+ * @dev: mbigen device this irq belong to.
+ * @dev_irq: virq number of this interrupt.
+ * @msi_irq: Corresponding msi irq number of this interrupt.
+ */
+struct mbigen_irq_data {
+	struct irq_priv_info	info;
+	struct mbigen_device	*dev;
+	unsigned int	dev_irq;
+	unsigned int	msi_irq;
+};
+
+/*
+ * global mbigen device list including all of the mbigen
+ * devices in this system
+ */
+static LIST_HEAD(mbigen_device_list);
+static DEFINE_SPINLOCK(mbigen_device_lock);
+
+static inline int get_mbigen_vec_reg_addr(u32 nid, u32 offset)
+{
+	return MBIGEN_NODE_ADDR_BASE(nid) + REG_MBIGEN_VEC_OFFSET
+	    + (offset * 4);
+}
+
+static struct mbigen_irq_data *get_mbigen_irq_data(struct mbigen_device *mgn_dev,
+						   struct irq_data *d)
+{
+	struct irq_priv_info *info;
+	u32 index;
+
+	index = GET_IRQ_INDEX(d->hwirq);
+	if (index < 0)
+		return NULL;
+
+	info = &mgn_dev->mgn_data[index].info;
+	info->index = index;
+	info->global_pin_offset = GET_IRQ_PIN_OFFSET(d->hwirq);
+	info->nid = info->global_pin_offset / IRQS_PER_MBIGEN_NODE;
+
+	info->local_pin_offset = (info->global_pin_offset % IRQS_PER_MBIGEN_NODE)
+						- RESERVED_IRQ_PER_MBIGEN_CHIP;
+
+	info->reg_offset = get_mbigen_vec_reg_addr(info->nid, info->local_pin_offset);
+
+	return &mgn_dev->mgn_data[index];
+}
+
+static int mbigen_set_affinity(struct irq_data *data,
+				const struct cpumask *mask_val,
+				bool force)
+{
+	struct mbigen_irq_data *mgn_irq_data = irq_data_get_irq_chip_data(data);
+	struct irq_chip *chip = irq_get_chip(mgn_irq_data->msi_irq);
+	struct irq_data *parent_d = irq_get_irq_data(mgn_irq_data->msi_irq);
+
+	if (chip && chip->irq_set_affinity)
+		return chip->irq_set_affinity(parent_d, mask_val, force);
+	else
+		return -EINVAL;
+}
+
+static void mbigen_mask_irq(struct irq_data *data)
+{
+	struct mbigen_irq_data *mgn_irq_data = irq_data_get_irq_chip_data(data);
+	struct irq_chip *chip = irq_get_chip(mgn_irq_data->msi_irq);
+	struct irq_data *parent_d = irq_get_irq_data(mgn_irq_data->msi_irq);
+
+	if (chip && chip->irq_mask)
+		return chip->irq_mask(parent_d);
+}
+
+static void mbigen_unmask_irq(struct irq_data *data)
+{
+	struct mbigen_irq_data *mgn_irq_data = irq_data_get_irq_chip_data(data);
+	struct irq_chip *chip = irq_get_chip(mgn_irq_data->msi_irq);
+	struct irq_data *parent_d = irq_get_irq_data(mgn_irq_data->msi_irq);
+
+	if (chip && chip->irq_unmask)
+		chip->irq_unmask(parent_d);
+}
+
+static void mbigen_eoi_irq(struct irq_data *data)
+{
+
+	struct mbigen_irq_data *mgn_irq_data = irq_data_get_irq_chip_data(data);
+	struct mbigen_device *mgn_dev = mgn_irq_data->dev;
+	struct irq_chip *chip = irq_get_chip(mgn_irq_data->msi_irq);
+	struct irq_data *parent_d = irq_get_irq_data(mgn_irq_data->msi_irq);
+	u32 pin_offset, ofst, mask;
+
+	pin_offset = mgn_irq_data->info.local_pin_offset;
+
+	ofst = pin_offset / 32 * 4;
+	mask = 1 << (pin_offset % 32);
+
+	writel_relaxed(mask, mgn_dev->base + ofst
+			+ REG_MBIGEN_CLEAR_OFFSET);
+
+	if (chip && chip->irq_eoi)
+		chip->irq_eoi(parent_d);
+}
+
+static struct irq_chip mbigen_irq_chip = {
+	.name = "mbigen-intc-v2",
+	.irq_mask = mbigen_mask_irq,
+	.irq_unmask = mbigen_unmask_irq,
+	.irq_eoi = mbigen_eoi_irq,
+	.irq_set_affinity = mbigen_set_affinity,
+};
+
+static int mbigen_domain_xlate(struct irq_domain *d,
+			       struct device_node *controller,
+			       const u32 *intspec, unsigned int intsize,
+			       unsigned long *out_hwirq,
+			       unsigned int *out_type)
+{
+
+	if (d->of_node != controller)
+		return -EINVAL;
+
+	if (intsize < 2)
+		return -EINVAL;
+
+	/* Compose the hwirq local to mbigen domain
+	 * intspec[0]: interrut pin offset
+	 * intspec[1]: index(start from 0)
+	 */
+	*out_hwirq = COMPOSE_MBIGEN_HWIRQ(intspec[1], intspec[0]);
+	*out_type = 0;
+
+	return 0;
+}
+
+static int mbigen_domain_map(struct irq_domain *d, unsigned int irq,
+				irq_hw_number_t hw)
+{
+	struct mbigen_device *mgn_dev = d->host_data;
+	struct mbigen_irq_data *mgn_irq_data;
+	struct irq_data *data = irq_get_irq_data(irq);
+
+	mgn_irq_data = get_mbigen_irq_data(mgn_dev, data);
+	if (!mgn_irq_data)
+		return -EINVAL;
+
+	mgn_irq_data->dev_irq = irq;
+	irq_set_chip_data(irq, mgn_irq_data);
+	irq_set_chip_and_handler(irq, &mbigen_irq_chip, handle_fasteoi_irq);
+
+	set_irq_flags(irq, IRQF_VALID);
+
+	return 0;
+}
+
+static struct irq_domain_ops mbigen_domain_ops = {
+	.xlate = mbigen_domain_xlate,
+	.map = mbigen_domain_map,
+};
+
+/*
+ * mbigen_device_init()- initial mbigen devices connected to
+ * mbigen chip as a interrupt controller
+ */
+static int __init mbigen_intc_of_init(struct device_node *node,
+				 struct device_node *parent)
+{
+	struct mbigen_device *mgn_dev;
+	struct irq_domain *domain;
+	struct mbigen_irq_data *mgn_irq_data;
+	u32 nvec;
+	int ret;
+
+	mgn_dev = kzalloc(sizeof(*mgn_dev), GFP_KERNEL);
+	if (!mgn_dev)
+		return -ENOMEM;
+
+	mgn_dev->node = node;
+
+	of_property_read_u32(node, "nr-interrupts", &nvec);
+	if (!nvec) {
+		ret = -EINVAL;
+		goto out_free_dev;
+	}
+
+	mgn_dev->nr_irqs = nvec;
+
+	mgn_irq_data = kcalloc(nvec, sizeof(*mgn_irq_data), GFP_KERNEL);
+	if (!mgn_irq_data) {
+		ret = -ENOMEM;
+		goto out_free_dev;
+	}
+
+	mgn_dev->mgn_data = mgn_irq_data;
+
+	domain = irq_domain_add_tree(node, &mbigen_domain_ops, mgn_dev);
+	if (!domain) {
+			ret = -ENOMEM;
+			goto out_free_data;
+	}
+	mgn_dev->domain = domain;
+
+	INIT_LIST_HEAD(&mgn_dev->global_entry);
+
+	/* add this mbigen device into a global list*/
+	spin_lock(&mbigen_device_lock);
+	list_add(&mgn_dev->global_entry, &mbigen_device_list);
+	spin_unlock(&mbigen_device_lock);
+
+	return 0;
+
+out_free_data:
+	kfree(mgn_dev->mgn_data);
+out_free_dev:
+	kfree(mgn_dev);
+	pr_err("mbigen-v2:failed to initialize mbigen device:%s (%d)\n",
+						node->full_name, ret);
+	return ret;
+}
+IRQCHIP_DECLARE(hisi_mbigen, "hisilicon,mbigen-intc-v2", mbigen_intc_of_init);
+
+MODULE_AUTHOR("Jun Ma <[e-mail]>");
+MODULE_AUTHOR("Yun Wu <[e-mail]>");
+MODULE_LICENSE("GPL");
+MODULE_DESCRIPTION("Hisilicon MBI Generator driver");