[v2,2/3] IRQ/Gic-V3: Change arm-gic-its to support the Mbigen interrupt

On 26/06/15 07:31, majun (F) wrote:
> Hi Thomas:
> 
> 在 2015/6/12 10:49, Ma Jun 写道:
> 
>> +static int its_mbigen_prepare(struct irq_domain *domain, struct mbi_desc *desc,
>> +								int hwirq, struct mbi_alloc_info *arg)
>> +{
>> +	struct its_node *its = domain->parent->host_data;
>> +	struct its_device *its_dev;
>> +	u32 dev_id;
>> +
>> +	dev_id = desc->msg_id;
>> +
>> +	its_dev = its_find_device(its, dev_id);
>> +	if (!its_dev) {
>> +		its_dev = its_create_device(its, dev_id, desc->lines);
>> +		if (!its_dev)
>> +			return -ENOMEM;
>> +	}
>> +
>> +	arg->scratchpad[0].ptr = its_dev;
>> +	arg->scratchpad[1].ptr = NULL;
>> +
>> +	arg->desc = desc;
>> +	arg->hwirq = hwirq;
>> +	return 0;
>> +}
>> +
>> +static struct mbigen_domain_ops its_mbigen_ops = {
>> +	.mbigen_prepare		= its_mbigen_prepare,
>> +};
>> +
>> +static struct mbigen_domain_info its_mbigen_domain_info = {
>> +	.ops	= &its_mbigen_ops,
>> +};
>> +
> 
> what's you opinion about the function 'its_mbigen_prepare' ?
> put this function in irq-gic-v3-its.c or move to mbigen driver ?
> 
> If I move this function to mbigen driver, some its fuctions
> (ex. its_find_device, its_create_device) and struct data (ex its_node)
> would be used in mbigen driver.

The prepare hook is very PCI specific so far, but could easily be turned into
something that is not. How about splitting it in two:


You can then keep your MBI stuff in a separate file, and call into 
its_msi_prepare.

> Now, all these functions and data structure are defined as static.
> to use them, I have to remove the 'static' definition and put them
> in a head file （ create a new head file).

I don't want to see these functions and structure leaking out of the
ITS code unless we're absolutely forced to do so.  The above code
shows you one possible way to solve the problem.

Thanks,

	M.

Hi Marc:

在 2015/6/26 16:44, Marc Zyngier 写道:
> 
> You can then keep your MBI stuff in a separate file, and call into 
> its_msi_prepare.
> 
Thanks for your good suggestion!
I have two questions:

Question 1:

I found the ‘its_msi_preapare ' defined without static.
So,I guess you mean I can call this fucntion directly
from mbigen driver, right?
or I need make the code likes below and leave these code in ITS?

static struct mbigen_domain_ops its_mbigen_ops = {
+	.mbigen_prepare	= its_msi_prepare,
};

static struct mbigen_domain_info its_mbigen_domain_info = {
	.ops	= &its_mbigen_ops,
};

Question 2:

@@ -1489,6 +1538,18 @@ static int its_probe(struct device_node *node, struct irq_domain *parent)
 		err = of_pci_msi_chip_add(&its->msi_chip);
 		if (err)
 			goto out_free_domains;
+
+		if (IS_ENABLED(CONFIG_MBIGEN_IRQ_DOMAIN)) {
+			its->mbi_chip.domain = its_mbigen_create_irq_domain(node,
+										&its_mbigen_domain_info,
+										its->domain);
+
+			if (!its->mbi_chip.domain) {
+				err = -ENOMEM;
+				pr_warn_once("ITS:no mbigen chip found\n");
+				goto out_free_mbigen;
+			}
+		}
 	}

 	spin_lock(&its_lock);
@@ -1497,6 +1558,9 @@ static int its_probe(struct device_node *node, struct irq_domain *parent)

 	return 0;

+out_free_mbigen:
+	if (its->mbi_chip.domain)
+		irq_domain_remove(its->mbi_chip.domain);
 out_free_domains:
 	if (its->msi_chip.domain)
 		irq_domain_remove(its->msi_chip.domain);

What's you opinion about the code above
Leave it in ITS or create the mbi irq domain in mbigen driver?
If I have to create mbi irq domain in mbigen driver,
I need a pointer of its domain.

For this problem, I think i can solve it by using its_nodes’
in mbigen driver *if*
[1] add a member " struct device_node *node" in 'struct its_node'
[2] in 'its_probe' function , add
	its->node = node;
[3] remove the static definition from  'static LIST_HEAD(its_nodes);'

How is you opinion?

Thansks again!



>> Now, all these functions and data structure are defined as static.
>> to use them, I have to remove the 'static' definition and put them
>> in a head file （ create a new head file).
> 
> I don't want to see these functions and structure leaking out of the
> ITS code unless we're absolutely forced to do so.  The above code
> shows you one possible way to solve the problem.
>

On 26/06/15 11:28, majun (F) wrote:
> 
> Hi Marc:
> 
> 在 2015/6/26 16:44, Marc Zyngier 写道:
>>
>> You can then keep your MBI stuff in a separate file, and call into 
>> its_msi_prepare.
>>
> Thanks for your good suggestion!
> I have two questions:
> 
> Question 1:
> 
> I found the ‘its_msi_preapare ' defined without static.
> So,I guess you mean I can call this fucntion directly
> from mbigen driver, right?

Yes. You can use it as part of your own msi_prepare function.

> or I need make the code likes below and leave these code in ITS?
> 
> static struct mbigen_domain_ops its_mbigen_ops = {
> +	.mbigen_prepare	= its_msi_prepare,
> };

This structure does not exist. Use the normal msi_domain_ops structure.

> 
> static struct mbigen_domain_info its_mbigen_domain_info = {
> 	.ops	= &its_mbigen_ops,
> };
> 
> Question 2:
> 
> @@ -1489,6 +1538,18 @@ static int its_probe(struct device_node *node, struct irq_domain *parent)
>  		err = of_pci_msi_chip_add(&its->msi_chip);
>  		if (err)
>  			goto out_free_domains;
> +
> +		if (IS_ENABLED(CONFIG_MBIGEN_IRQ_DOMAIN)) {
> +			its->mbi_chip.domain = its_mbigen_create_irq_domain(node,
> +										&its_mbigen_domain_info,
> +										its->domain);
> +
> +			if (!its->mbi_chip.domain) {
> +				err = -ENOMEM;
> +				pr_warn_once("ITS:no mbigen chip found\n");
> +				goto out_free_mbigen;
> +			}
> +		}
>  	}
> 
>  	spin_lock(&its_lock);
> @@ -1497,6 +1558,9 @@ static int its_probe(struct device_node *node, struct irq_domain *parent)
> 
>  	return 0;
> 
> +out_free_mbigen:
> +	if (its->mbi_chip.domain)
> +		irq_domain_remove(its->mbi_chip.domain);
>  out_free_domains:
>  	if (its->msi_chip.domain)
>  		irq_domain_remove(its->msi_chip.domain);
> 
> What's you opinion about the code above
> Leave it in ITS or create the mbi irq domain in mbigen driver?
> If I have to create mbi irq domain in mbigen driver,
> I need a pointer of its domain.
> 
> For this problem, I think i can solve it by using its_nodes’
> in mbigen driver *if*
> [1] add a member " struct device_node *node" in 'struct its_node'
> [2] in 'its_probe' function , add
> 	its->node = node;
> [3] remove the static definition from  'static LIST_HEAD(its_nodes);'
> 
> How is you opinion?

My opinion is that we need to be able to lookup the domain from the core
code without any of these hacks, and this is what I'm working on at the
moment. There is no way external code will be allowed to mess with the
internals of the ITS.

For the time being, just expose the domain with a helper (you can match
it with the of_node). In the long run, you should be able to look it up
directly from the domain list.

Thanks,

	M.

在 2015/6/26 18:40, Marc Zyngier 写道:
> 
> My opinion is that we need to be able to lookup the domain from the core
> code without any of these hacks, and this is what I'm working on at the
> moment. There is no way external code will be allowed to mess with the
> internals of the ITS.
> 
> For the time being, just expose the domain with a helper (you can match
> it with the of_node). 
Do you mean add a fucntion in ITS likes below:

struct irq_domain *get_its_domain(struct device_node *node)
{
	struct its_node *its = NULL;
	
	list_for_each_entry(its, &its_nodes, entry) {
		if(its->msi_chip.of_node == node)
			break;
	}

	return (its)?its->domain:NULL;
}

How about add a '.match ' member in its_domain_ops
just like:
	.match = get_its_domain;

So, I can use the fucntion 'irq_find_host' in mbigne driver

>In the long run, you should be able to look it up
> directly from the domain list.
> 
I think the domain list you said is 'irq_domain_list'
which defined in Irqdomain.c
static LIST_HEAD(irq_domain_list);

Thanks!

On 26/06/15 13:04, majun (F) wrote:
> 
> 
> 在 2015/6/26 18:40, Marc Zyngier 写道:
>>
>> My opinion is that we need to be able to lookup the domain from the core
>> code without any of these hacks, and this is what I'm working on at the
>> moment. There is no way external code will be allowed to mess with the
>> internals of the ITS.
>>
>> For the time being, just expose the domain with a helper (you can match
>> it with the of_node). 
> Do you mean add a fucntion in ITS likes below:
> 
> struct irq_domain *get_its_domain(struct device_node *node)
> {
> 	struct its_node *its = NULL;
> 	
> 	list_for_each_entry(its, &its_nodes, entry) {
> 		if(its->msi_chip.of_node == node)
> 			break;
> 	}
> 
> 	return (its)?its->domain:NULL;
> }

Yes.

> 
> How about add a '.match ' member in its_domain_ops
> just like:
> 	.match = get_its_domain;
> 
> So, I can use the fucntion 'irq_find_host' in mbigne driver

And that will only return the PCI/MSI domain, which is not of any help
to you.

At the moment, we register the PCI/MSI domain with the the of_node of
the ITS so that a PCI controller can match the its MSI controller, and
the ITS domain is completely anonymous (it doesn't have an of_node).

What I'm working on is a way to distinguish between several domains that
are identified by the same of_node, but cater for different bus types.
The current match function doesn't quite work for that case.

Thanks,

	M.