Re: [PATCH v2 2/3] IRQ/Gic-V3: Change arm-gic-its to support the Mbigen interrupt
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, _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.
--
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 v2 2/3] IRQ/Gic-V3: Change arm-gic-its to support the Mbigen interrupt
在 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, _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!
--
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 v2 2/3] IRQ/Gic-V3: Change arm-gic-its to support the Mbigen interrupt
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= _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(>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,
> +
> _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(_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.
--
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 v2 2/3] IRQ/Gic-V3: Change arm-gic-its to support the Mbigen interrupt
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= _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(>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,
+
_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(_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.
>
--
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 v2 2/3] IRQ/Gic-V3: Change arm-gic-its to support the Mbigen interrupt
On 23/06/15 10:29, Thomas Gleixner wrote: > On Tue, 23 Jun 2015, majun (F) wrote: >> 在 2015/6/19 7:52, Thomas Gleixner 写道: >>> On Mon, 15 Jun 2015, majun (F) wrote: 在 2015/6/12 18:48, Thomas Gleixner 写道: > Can you please provide a proper description of this mbigen chip and > explain WHY you think that it needs all this special hackery? >>> >>> You carefully avoided to provide a proper description of this mbigen >>> chip and how it needs to be integrated into the GIC/ITS whatever >>> scenario. >>> >> Mbigen means Message Based Interrupt Generator. >> Its a kind of interrupt controller 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 enought for so many perpherals. >> >> Mbigen is designed to fix this problem. >> >> Mbigen chip locates in ITS or outside of ITS. >> >> The working flow of Mbigen shows as below: >> >> external devices --> MBIGEN --->ITS >> >> The devices connect to Mbigen chip through wire connecting way. >> Mbigen detects and collectes the interrupts from the these devices. >> >> Then, Mbigen generats the MBI interrupts by writting the ITS >> Translator register. > > So it's nothing else than a non PCI based MSI implementation which > means it can simply use the generic MSI infrastructure and implement a > interrupt domain/chip which implements the MBI specific parts and has > the ITS as its parent domain. > > No hackery in ITS and no extra functionality in the core irq code. It > just can use the existing infrastructure. The only extra you need is a > proper way to retrieve the pointer to the ITS domain. Everything else > just falls in place. I may have a proposal for that. Stay tuned. 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 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= _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:
diff --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq-gic-v3-its.c
index 1b7e155..9a68c77 100644
--- a/drivers/irqchip/irq-gic-v3-its.c
+++ b/drivers/irqchip/irq-gic-v3-its.c
@@ -1200,49 +1200,54 @@ static int its_get_pci_alias(struct pci_dev *pdev, u16
alias, void *data)
return 0;
}
-static int its_msi_prepare(struct irq_domain *domain, struct device *dev,
- int nvec, msi_alloc_info_t *info)
+int its_msi_prepare(struct irq_domain *domain, u32 dev_id,
+ int nvec, msi_alloc_info_t *info)
{
- struct pci_dev *pdev;
struct its_node *its;
struct its_device *its_dev;
- struct its_pci_alias dev_alias;
- if (!dev_is_pci(dev))
- return -EINVAL;
-
- pdev = to_pci_dev(dev);
- dev_alias.pdev = pdev;
- dev_alias.count = nvec;
-
- pci_for_each_dma_alias(pdev, its_get_pci_alias, _alias);
its = domain->parent->host_data;
-
- its_dev = its_find_device(its, dev_alias.dev_id);
+ its_dev = its_find_device(its, dev_id);
if (its_dev) {
/*
* We already have seen this ID, probably through
* another alias (PCI bridge of some sort). No need to
* create the device.
*/
- dev_dbg(dev, "Reusing ITT for devID %x\n", dev_alias.dev_id);
+ pr_debug("Reusing ITT for devID %x\n", dev_id);
goto out;
}
- its_dev = its_create_device(its, dev_alias.dev_id, dev_alias.count);
+ its_dev = its_create_device(its, dev_id, nvec);
if (!its_dev)
return -ENOMEM;
- dev_dbg(>dev, "ITT %d entries, %d bits\n",
- dev_alias.count, ilog2(dev_alias.count));
+ pr_debug("ITT %d entries, %d bits\n", nvec, ilog2(nvec));
out:
info->scratchpad[0].ptr = its_dev;
- info->scratchpad[1].ptr = dev;
return 0;
}
+static int its_pci_msi_prepare(struct irq_domain *domain, struct device *dev,
+ int nvec, msi_alloc_info_t *info)
+{
+ struct pci_dev *pdev;
+ struct its_pci_alias dev_alias;
+
+ if (!dev_is_pci(dev))
+ return -EINVAL;
+
+ pdev = to_pci_dev(dev);
+ dev_alias.pdev = pdev;
+ dev_alias.count = nvec;
+
+ pci_for_each_dma_alias(pdev, its_get_pci_alias, _alias);
+
+ return its_msi_prepare(domain, dev_alias.dev_id, dev_alias.count, info);
+}
+
static struct msi_domain_ops its_pci_msi_ops = {
- .msi_prepare= its_msi_prepare,
+ .msi_prepare= its_pci_msi_prepare,
};
static struct msi_domain_info its_pci_msi_domain_info = {
@@ -1287,8 +1292,9 @@ static int its_irq_domain_alloc(struct irq_domain
*domain, unsigned int virq,
irq_domain_set_hwirq_and_chip(domain, virq + i,
hwirq, _irq_chip, its_dev);
- dev_dbg(info->scratchpad[1].ptr, "ID:%d pID:%d vID:%d\n",
- (int)(hwirq - its_dev->lpi_base), (int)hwirq, virq + i);
+ pr_debug("ID:%d pID:%d vID:%d\n",
+(int)(hwirq - its_dev->lpi_base),
+(int)hwirq, virq + i);
}
return 0;
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'
Re: [PATCH v2 2/3] IRQ/Gic-V3: Change arm-gic-its to support the Mbigen interrupt
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= _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.
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'm not sure which way is better .
--
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 v2 2/3] IRQ/Gic-V3: Change arm-gic-its to support the Mbigen interrupt
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.
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'm not sure which way is better .
--
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 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:
diff --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq-gic-v3-its.c
index 1b7e155..9a68c77 100644
--- a/drivers/irqchip/irq-gic-v3-its.c
+++ b/drivers/irqchip/irq-gic-v3-its.c
@@ -1200,49 +1200,54 @@ static int its_get_pci_alias(struct pci_dev *pdev, u16
alias, void *data)
return 0;
}
-static int its_msi_prepare(struct irq_domain *domain, struct device *dev,
- int nvec, msi_alloc_info_t *info)
+int its_msi_prepare(struct irq_domain *domain, u32 dev_id,
+ int nvec, msi_alloc_info_t *info)
{
- struct pci_dev *pdev;
struct its_node *its;
struct its_device *its_dev;
- struct its_pci_alias dev_alias;
- if (!dev_is_pci(dev))
- return -EINVAL;
-
- pdev = to_pci_dev(dev);
- dev_alias.pdev = pdev;
- dev_alias.count = nvec;
-
- pci_for_each_dma_alias(pdev, its_get_pci_alias, dev_alias);
its = domain-parent-host_data;
-
- its_dev = its_find_device(its, dev_alias.dev_id);
+ its_dev = its_find_device(its, dev_id);
if (its_dev) {
/*
* We already have seen this ID, probably through
* another alias (PCI bridge of some sort). No need to
* create the device.
*/
- dev_dbg(dev, Reusing ITT for devID %x\n, dev_alias.dev_id);
+ pr_debug(Reusing ITT for devID %x\n, dev_id);
goto out;
}
- its_dev = its_create_device(its, dev_alias.dev_id, dev_alias.count);
+ its_dev = its_create_device(its, dev_id, nvec);
if (!its_dev)
return -ENOMEM;
- dev_dbg(pdev-dev, ITT %d entries, %d bits\n,
- dev_alias.count, ilog2(dev_alias.count));
+ pr_debug(ITT %d entries, %d bits\n, nvec, ilog2(nvec));
out:
info-scratchpad[0].ptr = its_dev;
- info-scratchpad[1].ptr = dev;
return 0;
}
+static int its_pci_msi_prepare(struct irq_domain *domain, struct device *dev,
+ int nvec, msi_alloc_info_t *info)
+{
+ struct pci_dev *pdev;
+ struct its_pci_alias dev_alias;
+
+ if (!dev_is_pci(dev))
+ return -EINVAL;
+
+ pdev = to_pci_dev(dev);
+ dev_alias.pdev = pdev;
+ dev_alias.count = nvec;
+
+ pci_for_each_dma_alias(pdev, its_get_pci_alias, dev_alias);
+
+ return its_msi_prepare(domain, dev_alias.dev_id, dev_alias.count, info);
+}
+
static struct msi_domain_ops its_pci_msi_ops = {
- .msi_prepare= its_msi_prepare,
+ .msi_prepare= its_pci_msi_prepare,
};
static struct msi_domain_info its_pci_msi_domain_info = {
@@ -1287,8 +1292,9 @@ static int its_irq_domain_alloc(struct irq_domain
*domain, unsigned int virq,
irq_domain_set_hwirq_and_chip(domain, virq + i,
hwirq, its_irq_chip, its_dev);
- dev_dbg(info-scratchpad[1].ptr, ID:%d pID:%d vID:%d\n,
- (int)(hwirq - its_dev-lpi_base), (int)hwirq, virq + i);
+ pr_debug(ID:%d pID:%d vID:%d\n,
+(int)(hwirq - its_dev-lpi_base),
+(int)hwirq, virq + i);
}
return 0;
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
Re: [PATCH v2 2/3] IRQ/Gic-V3: Change arm-gic-its to support the Mbigen interrupt
On 23/06/15 10:29, Thomas Gleixner wrote: On Tue, 23 Jun 2015, majun (F) wrote: 在 2015/6/19 7:52, Thomas Gleixner 写道: On Mon, 15 Jun 2015, majun (F) wrote: 在 2015/6/12 18:48, Thomas Gleixner 写道: Can you please provide a proper description of this mbigen chip and explain WHY you think that it needs all this special hackery? You carefully avoided to provide a proper description of this mbigen chip and how it needs to be integrated into the GIC/ITS whatever scenario. Mbigen means Message Based Interrupt Generator. Its a kind of interrupt controller 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 enought for so many perpherals. Mbigen is designed to fix this problem. Mbigen chip locates in ITS or outside of ITS. The working flow of Mbigen shows as below: external devices -- MBIGEN ---ITS The devices connect to Mbigen chip through wire connecting way. Mbigen detects and collectes the interrupts from the these devices. Then, Mbigen generats the MBI interrupts by writting the ITS Translator register. So it's nothing else than a non PCI based MSI implementation which means it can simply use the generic MSI infrastructure and implement a interrupt domain/chip which implements the MBI specific parts and has the ITS as its parent domain. No hackery in ITS and no extra functionality in the core irq code. It just can use the existing infrastructure. The only extra you need is a proper way to retrieve the pointer to the ITS domain. Everything else just falls in place. I may have a proposal for that. Stay tuned. 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 v2 2/3] IRQ/Gic-V3: Change arm-gic-its to support the Mbigen interrupt
在 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!
--
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 v2 2/3] IRQ/Gic-V3: Change arm-gic-its to support the Mbigen interrupt
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.
--
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 v2 2/3] IRQ/Gic-V3: Change arm-gic-its to support the Mbigen interrupt
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.
--
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 v2 2/3] IRQ/Gic-V3: Change arm-gic-its to support the Mbigen interrupt
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.
--
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 v2 2/3] IRQ/Gic-V3: Change arm-gic-its to support the Mbigen interrupt
On Tue, 23 Jun 2015, majun (F) wrote: > 在 2015/6/19 7:52, Thomas Gleixner 写道: > > On Mon, 15 Jun 2015, majun (F) wrote: > >> 在 2015/6/12 18:48, Thomas Gleixner 写道: > >>> Can you please provide a proper description of this mbigen chip and > >>> explain WHY you think that it needs all this special hackery? > > > > You carefully avoided to provide a proper description of this mbigen > > chip and how it needs to be integrated into the GIC/ITS whatever > > scenario. > > > Mbigen means Message Based Interrupt Generator. > Its a kind of interrupt controller 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 enought for so many perpherals. > > Mbigen is designed to fix this problem. > > Mbigen chip locates in ITS or outside of ITS. > > The working flow of Mbigen shows as below: > > external devices --> MBIGEN --->ITS > > The devices connect to Mbigen chip through wire connecting way. > Mbigen detects and collectes the interrupts from the these devices. > > Then, Mbigen generats the MBI interrupts by writting the ITS > Translator register. So it's nothing else than a non PCI based MSI implementation which means it can simply use the generic MSI infrastructure and implement a interrupt domain/chip which implements the MBI specific parts and has the ITS as its parent domain. No hackery in ITS and no extra functionality in the core irq code. It just can use the existing infrastructure. The only extra you need is a proper way to retrieve the pointer to the ITS domain. Everything else just falls in place. Thanks, tglx
Re: [PATCH v2 2/3] IRQ/Gic-V3: Change arm-gic-its to support the Mbigen interrupt
在 2015/6/19 7:52, Thomas Gleixner 写道: > On Mon, 15 Jun 2015, majun (F) wrote: >> 在 2015/6/12 18:48, Thomas Gleixner 写道: >>> Can you please provide a proper description of this mbigen chip and >>> explain WHY you think that it needs all this special hackery? > > You carefully avoided to provide a proper description of this mbigen > chip and how it needs to be integrated into the GIC/ITS whatever > scenario. > Mbigen means Message Based Interrupt Generator. Its a kind of interrupt controller 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 enought for so many perpherals. Mbigen is designed to fix this problem. Mbigen chip locates in ITS or outside of ITS. The working flow of Mbigen shows as below: external devices --> MBIGEN --->ITS The devices connect to Mbigen chip through wire connecting way. Mbigen detects and collectes the interrupts from the these devices. Then, Mbigen generats the MBI interrupts by writting the ITS Translator register. -- 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 v2 2/3] IRQ/Gic-V3: Change arm-gic-its to support the Mbigen interrupt
On Tue, 23 Jun 2015, majun (F) wrote: 在 2015/6/19 7:52, Thomas Gleixner 写道: On Mon, 15 Jun 2015, majun (F) wrote: 在 2015/6/12 18:48, Thomas Gleixner 写道: Can you please provide a proper description of this mbigen chip and explain WHY you think that it needs all this special hackery? You carefully avoided to provide a proper description of this mbigen chip and how it needs to be integrated into the GIC/ITS whatever scenario. Mbigen means Message Based Interrupt Generator. Its a kind of interrupt controller 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 enought for so many perpherals. Mbigen is designed to fix this problem. Mbigen chip locates in ITS or outside of ITS. The working flow of Mbigen shows as below: external devices -- MBIGEN ---ITS The devices connect to Mbigen chip through wire connecting way. Mbigen detects and collectes the interrupts from the these devices. Then, Mbigen generats the MBI interrupts by writting the ITS Translator register. So it's nothing else than a non PCI based MSI implementation which means it can simply use the generic MSI infrastructure and implement a interrupt domain/chip which implements the MBI specific parts and has the ITS as its parent domain. No hackery in ITS and no extra functionality in the core irq code. It just can use the existing infrastructure. The only extra you need is a proper way to retrieve the pointer to the ITS domain. Everything else just falls in place. Thanks, tglx
Re: [PATCH v2 2/3] IRQ/Gic-V3: Change arm-gic-its to support the Mbigen interrupt
在 2015/6/19 7:52, Thomas Gleixner 写道: On Mon, 15 Jun 2015, majun (F) wrote: 在 2015/6/12 18:48, Thomas Gleixner 写道: Can you please provide a proper description of this mbigen chip and explain WHY you think that it needs all this special hackery? You carefully avoided to provide a proper description of this mbigen chip and how it needs to be integrated into the GIC/ITS whatever scenario. Mbigen means Message Based Interrupt Generator. Its a kind of interrupt controller 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 enought for so many perpherals. Mbigen is designed to fix this problem. Mbigen chip locates in ITS or outside of ITS. The working flow of Mbigen shows as below: external devices -- MBIGEN ---ITS The devices connect to Mbigen chip through wire connecting way. Mbigen detects and collectes the interrupts from the these devices. Then, Mbigen generats the MBI interrupts by writting the ITS Translator register. -- 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 v2 2/3] IRQ/Gic-V3: Change arm-gic-its to support the Mbigen interrupt
On Mon, 15 Jun 2015, majun (F) wrote: > 在 2015/6/12 18:48, Thomas Gleixner 写道: > > Can you please provide a proper description of this mbigen chip and > > explain WHY you think that it needs all this special hackery? You carefully avoided to provide a proper description of this mbigen chip and how it needs to be integrated into the GIC/ITS whatever scenario. Thanks, tglx
Re: [PATCH v2 2/3] IRQ/Gic-V3: Change arm-gic-its to support the Mbigen interrupt
On Mon, 15 Jun 2015, majun (F) wrote: 在 2015/6/12 18:48, Thomas Gleixner 写道: Can you please provide a proper description of this mbigen chip and explain WHY you think that it needs all this special hackery? You carefully avoided to provide a proper description of this mbigen chip and how it needs to be integrated into the GIC/ITS whatever scenario. Thanks, tglx
Re: [PATCH v2 2/3] IRQ/Gic-V3: Change arm-gic-its to support the Mbigen interrupt
在 2015/6/12 18:48, Thomas Gleixner 写道:
> On Fri, 12 Jun 2015, Ma Jun wrote:
>
>> This patch is applied to support the mbigen interrupt.
>>
>> As a kind of MSI interrupt controller, the mbigen is used as a child
>> domain of ITS domain just like PCI devices.
>> So the arm-gic-v3-its and related files are changed.
>>
>> The chip.c is also changed to check irq_ach before it called.
>
> This patch wants to be split into several:
>
> 1) Changes to the core code
>
> 2) New functionality in the core code
>
> 2) Changes to gic-v3-its
>
> And all patches require proper changelogs which explain WHY these
> changes are necessary.
>
> We can see which files are changed from the diffstat and the patch
> ourself. So no point to mention this in the changelog.
>
> But we cannot figure out from looking at the code WHY you think that
> your approach to solve the problem is the right one.
>
>> void irq_chip_ack_parent(struct irq_data *data)
>> {
>> data = data->parent_data;
>> -data->chip->irq_ack(data);
>> +if (data->chip->irq_ack)
>> +data->chip->irq_ack(data);
>
> Why is this required? Just because? Again, you fail to provide a
> rationale for the changes to the irq_chip*parent() functions.
>
> Why would you call irq_chip_ack_parent() if that parent does not
> provide the required functionality in the first place?
>
Yes, this is not a necessary callback. I will remove this callback
from mbigen driver.
>> /*
>> @@ -363,6 +364,9 @@ struct irq_chip {
>> int (*irq_request_resources)(struct irq_data *data);
>> void(*irq_release_resources)(struct irq_data *data);
>>
>> +void(*irq_compose_mbigen_msg)(struct irq_data *data, struct
>> mbigen_msg *msg);
>> +void(*irq_write_mbigen_msg)(struct irq_data *data, struct
>> mbigen_msg *msg);
>> +
>
> What's so special about mbigen to justify extra callbacks which just
> bloat the data structure for everyone. Why are the msi callbacks not
> sufficient?
>
> MBI is just another variant of MSI, right?
>
yes,MBI is a kind of MSI which used for non-pci devices.
According to Marc's advice, the irq hierachy structure
in my patch likes below:
non-pci devices-->mbigen-->its-->gic
pci devices -->msi __/
Eventhough the function *irq_compose_mbigen_msg does
the same thing as *irq_chip_compose_msi_msg, I still
added this function. Because I don't want mix the code
used by msi(pci devices) with the code used by mbigen.
> struct mbigen_msg {
>u32 address_lo;
>u32 address_hi;
>u32 data;
> };
>
> struct mbigen_msg is just a mindless copy of struct msi_msg:
>
> struct msi_msg {
> u32 address_lo; /* low 32 bits of msi message address */
> u32 address_hi; /* high 32 bits of msi message address */
> u32 data; /* 16 bits of msi message data */
> };
>
> So what's the point of this?
>
Based on the same reason, I also added structure mbigen_msg for
mbigen using.
>> void(*irq_compose_msi_msg)(struct irq_data *data, struct
>> msi_msg *msg);
>> void(*irq_write_msi_msg)(struct irq_data *data, struct
>> msi_msg *msg);
>>
>
>> +
>> +/**
>> + * irq_chip_compose_mbigen_msg - Componse mbigen message for a mbigen irq
>> chip
>> + * @data: Pointer to interrupt specific data
>> + * @msg:Pointer to the mbigen message
>> + *
>> + * For hierarchical domains we find the first chip in the hierarchy
>> + * which implements the irq_compose_mbigen_msg callback. For non
>> + * hierarchical we use the top level chip.
>> + */
>> +
>> +int irq_chip_compose_mbigen_msg(struct irq_data *data, struct mbigen_msg
>> *msg)
>> +{
>> +struct irq_data *pos = NULL;
>> +
>> +#ifdef CONFIG_IRQ_DOMAIN_HIERARCHY
>> +for (; data; data = data->parent_data)
>> +#endif
>> +if (data->chip && data->chip->irq_compose_mbigen_msg)
>> +pos = data;
>> +if (!pos)
>> +return -ENOSYS;
>> +
>> +pos->chip->irq_compose_mbigen_msg(pos, msg);
>> +
>> +return 0;
>> +}
>
> Again, this is a completely useless copy of irq_chip_compose_msi_msg().
> Why can't you just use the existing callbacks and use struct msi_msg
> for your special chip?
>
As mentioned before, to avoid using the code of msi, i added this
function.Because they are different domain.
If you don't mind, I can use the irq_chip_compose_msi_msg function in
mbigen driver instead of irq_chip_compose_mbigen_msg.
> And w/o looking at the mbigen code in detail, I bet it's nothing else
> than MSI for non PCI devices and contains tons of redundant and copied
> code, right?
>
> Can you please provide a proper description of this mbigen chip and
> explain WHY you think that it needs all this special hackery?
>
> Thanks,
>
> tglx
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majord...@vger.kernel.org
> More
Re: [PATCH v2 2/3] IRQ/Gic-V3: Change arm-gic-its to support the Mbigen interrupt
在 2015/6/12 18:48, Thomas Gleixner 写道:
On Fri, 12 Jun 2015, Ma Jun wrote:
This patch is applied to support the mbigen interrupt.
As a kind of MSI interrupt controller, the mbigen is used as a child
domain of ITS domain just like PCI devices.
So the arm-gic-v3-its and related files are changed.
The chip.c is also changed to check irq_ach before it called.
This patch wants to be split into several:
1) Changes to the core code
2) New functionality in the core code
2) Changes to gic-v3-its
And all patches require proper changelogs which explain WHY these
changes are necessary.
We can see which files are changed from the diffstat and the patch
ourself. So no point to mention this in the changelog.
But we cannot figure out from looking at the code WHY you think that
your approach to solve the problem is the right one.
void irq_chip_ack_parent(struct irq_data *data)
{
data = data-parent_data;
-data-chip-irq_ack(data);
+if (data-chip-irq_ack)
+data-chip-irq_ack(data);
Why is this required? Just because? Again, you fail to provide a
rationale for the changes to the irq_chip*parent() functions.
Why would you call irq_chip_ack_parent() if that parent does not
provide the required functionality in the first place?
Yes, this is not a necessary callback. I will remove this callback
from mbigen driver.
/*
@@ -363,6 +364,9 @@ struct irq_chip {
int (*irq_request_resources)(struct irq_data *data);
void(*irq_release_resources)(struct irq_data *data);
+void(*irq_compose_mbigen_msg)(struct irq_data *data, struct
mbigen_msg *msg);
+void(*irq_write_mbigen_msg)(struct irq_data *data, struct
mbigen_msg *msg);
+
What's so special about mbigen to justify extra callbacks which just
bloat the data structure for everyone. Why are the msi callbacks not
sufficient?
MBI is just another variant of MSI, right?
yes,MBI is a kind of MSI which used for non-pci devices.
According to Marc's advice, the irq hierachy structure
in my patch likes below:
non-pci devices--mbigen--its--gic
pci devices --msi __/
Eventhough the function *irq_compose_mbigen_msg does
the same thing as *irq_chip_compose_msi_msg, I still
added this function. Because I don't want mix the code
used by msi(pci devices) with the code used by mbigen.
struct mbigen_msg {
u32 address_lo;
u32 address_hi;
u32 data;
};
struct mbigen_msg is just a mindless copy of struct msi_msg:
struct msi_msg {
u32 address_lo; /* low 32 bits of msi message address */
u32 address_hi; /* high 32 bits of msi message address */
u32 data; /* 16 bits of msi message data */
};
So what's the point of this?
Based on the same reason, I also added structure mbigen_msg for
mbigen using.
void(*irq_compose_msi_msg)(struct irq_data *data, struct
msi_msg *msg);
void(*irq_write_msi_msg)(struct irq_data *data, struct
msi_msg *msg);
+
+/**
+ * irq_chip_compose_mbigen_msg - Componse mbigen message for a mbigen irq
chip
+ * @data: Pointer to interrupt specific data
+ * @msg:Pointer to the mbigen message
+ *
+ * For hierarchical domains we find the first chip in the hierarchy
+ * which implements the irq_compose_mbigen_msg callback. For non
+ * hierarchical we use the top level chip.
+ */
+
+int irq_chip_compose_mbigen_msg(struct irq_data *data, struct mbigen_msg
*msg)
+{
+struct irq_data *pos = NULL;
+
+#ifdef CONFIG_IRQ_DOMAIN_HIERARCHY
+for (; data; data = data-parent_data)
+#endif
+if (data-chip data-chip-irq_compose_mbigen_msg)
+pos = data;
+if (!pos)
+return -ENOSYS;
+
+pos-chip-irq_compose_mbigen_msg(pos, msg);
+
+return 0;
+}
Again, this is a completely useless copy of irq_chip_compose_msi_msg().
Why can't you just use the existing callbacks and use struct msi_msg
for your special chip?
As mentioned before, to avoid using the code of msi, i added this
function.Because they are different domain.
If you don't mind, I can use the irq_chip_compose_msi_msg function in
mbigen driver instead of irq_chip_compose_mbigen_msg.
And w/o looking at the mbigen code in detail, I bet it's nothing else
than MSI for non PCI devices and contains tons of redundant and copied
code, right?
Can you please provide a proper description of this mbigen chip and
explain WHY you think that it needs all this special hackery?
Thanks,
tglx
--
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/
.
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a
Re: [PATCH v2 2/3] IRQ/Gic-V3: Change arm-gic-its to support the Mbigen interrupt
On Fri, 12 Jun 2015, Ma Jun wrote:
> This patch is applied to support the mbigen interrupt.
>
> As a kind of MSI interrupt controller, the mbigen is used as a child
> domain of ITS domain just like PCI devices.
> So the arm-gic-v3-its and related files are changed.
>
> The chip.c is also changed to check irq_ach before it called.
This patch wants to be split into several:
1) Changes to the core code
2) New functionality in the core code
2) Changes to gic-v3-its
And all patches require proper changelogs which explain WHY these
changes are necessary.
We can see which files are changed from the diffstat and the patch
ourself. So no point to mention this in the changelog.
But we cannot figure out from looking at the code WHY you think that
your approach to solve the problem is the right one.
> void irq_chip_ack_parent(struct irq_data *data)
> {
> data = data->parent_data;
> - data->chip->irq_ack(data);
> + if (data->chip->irq_ack)
> + data->chip->irq_ack(data);
Why is this required? Just because? Again, you fail to provide a
rationale for the changes to the irq_chip*parent() functions.
Why would you call irq_chip_ack_parent() if that parent does not
provide the required functionality in the first place?
> /*
> @@ -363,6 +364,9 @@ struct irq_chip {
> int (*irq_request_resources)(struct irq_data *data);
> void(*irq_release_resources)(struct irq_data *data);
>
> + void(*irq_compose_mbigen_msg)(struct irq_data *data, struct
> mbigen_msg *msg);
> + void(*irq_write_mbigen_msg)(struct irq_data *data, struct
> mbigen_msg *msg);
> +
What's so special about mbigen to justify extra callbacks which just
bloat the data structure for everyone. Why are the msi callbacks not
sufficient?
MBI is just another variant of MSI, right?
struct mbigen_msg {
u32 address_lo;
u32 address_hi;
u32 data;
};
struct mbigen_msg is just a mindless copy of struct msi_msg:
struct msi_msg {
u32 address_lo; /* low 32 bits of msi message address */
u32 address_hi; /* high 32 bits of msi message address */
u32 data; /* 16 bits of msi message data */
};
So what's the point of this?
> void(*irq_compose_msi_msg)(struct irq_data *data, struct
> msi_msg *msg);
> void(*irq_write_msi_msg)(struct irq_data *data, struct
> msi_msg *msg);
>
> +
> +/**
> + * irq_chip_compose_mbigen_msg - Componse mbigen message for a mbigen irq
> chip
> + * @data:Pointer to interrupt specific data
> + * @msg: Pointer to the mbigen message
> + *
> + * For hierarchical domains we find the first chip in the hierarchy
> + * which implements the irq_compose_mbigen_msg callback. For non
> + * hierarchical we use the top level chip.
> + */
> +
> +int irq_chip_compose_mbigen_msg(struct irq_data *data, struct mbigen_msg
> *msg)
> +{
> + struct irq_data *pos = NULL;
> +
> +#ifdef CONFIG_IRQ_DOMAIN_HIERARCHY
> + for (; data; data = data->parent_data)
> +#endif
> + if (data->chip && data->chip->irq_compose_mbigen_msg)
> + pos = data;
> + if (!pos)
> + return -ENOSYS;
> +
> + pos->chip->irq_compose_mbigen_msg(pos, msg);
> +
> + return 0;
> +}
Again, this is a completely useless copy of irq_chip_compose_msi_msg().
Why can't you just use the existing callbacks and use struct msi_msg
for your special chip?
And w/o looking at the mbigen code in detail, I bet it's nothing else
than MSI for non PCI devices and contains tons of redundant and copied
code, right?
Can you please provide a proper description of this mbigen chip and
explain WHY you think that it needs all this special hackery?
Thanks,
tglx
--
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 v2 2/3] IRQ/Gic-V3: Change arm-gic-its to support the Mbigen interrupt
On Fri, 12 Jun 2015, Ma Jun wrote:
This patch is applied to support the mbigen interrupt.
As a kind of MSI interrupt controller, the mbigen is used as a child
domain of ITS domain just like PCI devices.
So the arm-gic-v3-its and related files are changed.
The chip.c is also changed to check irq_ach before it called.
This patch wants to be split into several:
1) Changes to the core code
2) New functionality in the core code
2) Changes to gic-v3-its
And all patches require proper changelogs which explain WHY these
changes are necessary.
We can see which files are changed from the diffstat and the patch
ourself. So no point to mention this in the changelog.
But we cannot figure out from looking at the code WHY you think that
your approach to solve the problem is the right one.
void irq_chip_ack_parent(struct irq_data *data)
{
data = data-parent_data;
- data-chip-irq_ack(data);
+ if (data-chip-irq_ack)
+ data-chip-irq_ack(data);
Why is this required? Just because? Again, you fail to provide a
rationale for the changes to the irq_chip*parent() functions.
Why would you call irq_chip_ack_parent() if that parent does not
provide the required functionality in the first place?
/*
@@ -363,6 +364,9 @@ struct irq_chip {
int (*irq_request_resources)(struct irq_data *data);
void(*irq_release_resources)(struct irq_data *data);
+ void(*irq_compose_mbigen_msg)(struct irq_data *data, struct
mbigen_msg *msg);
+ void(*irq_write_mbigen_msg)(struct irq_data *data, struct
mbigen_msg *msg);
+
What's so special about mbigen to justify extra callbacks which just
bloat the data structure for everyone. Why are the msi callbacks not
sufficient?
MBI is just another variant of MSI, right?
struct mbigen_msg {
u32 address_lo;
u32 address_hi;
u32 data;
};
struct mbigen_msg is just a mindless copy of struct msi_msg:
struct msi_msg {
u32 address_lo; /* low 32 bits of msi message address */
u32 address_hi; /* high 32 bits of msi message address */
u32 data; /* 16 bits of msi message data */
};
So what's the point of this?
void(*irq_compose_msi_msg)(struct irq_data *data, struct
msi_msg *msg);
void(*irq_write_msi_msg)(struct irq_data *data, struct
msi_msg *msg);
+
+/**
+ * irq_chip_compose_mbigen_msg - Componse mbigen message for a mbigen irq
chip
+ * @data:Pointer to interrupt specific data
+ * @msg: Pointer to the mbigen message
+ *
+ * For hierarchical domains we find the first chip in the hierarchy
+ * which implements the irq_compose_mbigen_msg callback. For non
+ * hierarchical we use the top level chip.
+ */
+
+int irq_chip_compose_mbigen_msg(struct irq_data *data, struct mbigen_msg
*msg)
+{
+ struct irq_data *pos = NULL;
+
+#ifdef CONFIG_IRQ_DOMAIN_HIERARCHY
+ for (; data; data = data-parent_data)
+#endif
+ if (data-chip data-chip-irq_compose_mbigen_msg)
+ pos = data;
+ if (!pos)
+ return -ENOSYS;
+
+ pos-chip-irq_compose_mbigen_msg(pos, msg);
+
+ return 0;
+}
Again, this is a completely useless copy of irq_chip_compose_msi_msg().
Why can't you just use the existing callbacks and use struct msi_msg
for your special chip?
And w/o looking at the mbigen code in detail, I bet it's nothing else
than MSI for non PCI devices and contains tons of redundant and copied
code, right?
Can you please provide a proper description of this mbigen chip and
explain WHY you think that it needs all this special hackery?
Thanks,
tglx
--
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/
