Re: [RFC v1 02/11] genirq: Move field 'node' from struct irq_data into struct irq_common_data
On 2015/5/8 10:29, Yun Wu (Abel) wrote:
> Hi Gerry,
> On 2015/5/4 11:15, Jiang Liu wrote:
>
>> NUMA node information is per-irq instead of per-irqchip, so move it into
>> struct irq_common_data.
>>
>> Signed-off-by: Jiang Liu
>> ---
>> arch/sh/kernel/irq.c |2 +-
>> arch/x86/kernel/apic/vector.c |8
>> arch/x86/platform/uv/uv_irq.c |2 +-
>> include/linux/irq.h | 20 ++--
>> kernel/irq/internals.h|5 +
>> kernel/irq/irqdesc.c | 10 ++
>> kernel/irq/irqdomain.c|4 ++--
>> kernel/irq/manage.c |2 +-
>> kernel/irq/proc.c |2 +-
>> 9 files changed, 35 insertions(+), 20 deletions(-)
>>
>> diff --git a/arch/sh/kernel/irq.c b/arch/sh/kernel/irq.c
>> index eb10ff84015c..8dc677cc136b 100644
>> --- a/arch/sh/kernel/irq.c
>> +++ b/arch/sh/kernel/irq.c
>> @@ -227,7 +227,7 @@ void migrate_irqs(void)
>> for_each_active_irq(irq) {
>> struct irq_data *data = irq_get_irq_data(irq);
>>
>> -if (data->node == cpu) {
>> +if (irq_data_get_node(data) == cpu) {
>> unsigned int newcpu = cpumask_any_and(data->affinity,
>>cpu_online_mask);
>> if (newcpu >= nr_cpu_ids) {
>> diff --git a/arch/x86/kernel/apic/vector.c b/arch/x86/kernel/apic/vector.c
>> index 96ce5068a926..983bea2a09ce 100644
>> --- a/arch/x86/kernel/apic/vector.c
>> +++ b/arch/x86/kernel/apic/vector.c
>> @@ -345,7 +345,7 @@ static int x86_vector_alloc_irqs(struct irq_domain
>> *domain, unsigned int virq,
>> struct irq_alloc_info *info = arg;
>> struct apic_chip_data *data;
>> struct irq_data *irq_data;
>> -int i, err;
>> +int i, err, node;
>>
>> if (disable_apic)
>> return -ENXIO;
>> @@ -357,12 +357,13 @@ static int x86_vector_alloc_irqs(struct irq_domain
>> *domain, unsigned int virq,
>> for (i = 0; i < nr_irqs; i++) {
>> irq_data = irq_domain_get_irq_data(domain, virq + i);
>> BUG_ON(!irq_data);
>> +node = irq_data_get_node(irq_data);
>> #ifdef CONFIG_X86_IO_APIC
>> if (virq + i < nr_legacy_irqs() && legacy_irq_data[virq + i])
>> data = legacy_irq_data[virq + i];
>> else
>> #endif
>> -data = alloc_apic_chip_data(irq_data->node);
>> +data = alloc_apic_chip_data(node);
>> if (!data) {
>> err = -ENOMEM;
>> goto error;
>> @@ -371,8 +372,7 @@ static int x86_vector_alloc_irqs(struct irq_domain
>> *domain, unsigned int virq,
>> irq_data->chip = _controller;
>> irq_data->chip_data = data;
>> irq_data->hwirq = virq + i;
>> -err = assign_irq_vector_policy(virq, irq_data->node, data,
>> - info);
>> +err = assign_irq_vector_policy(virq, node, data, info);
>> if (err)
>> goto error;
>> }
>> diff --git a/arch/x86/platform/uv/uv_irq.c b/arch/x86/platform/uv/uv_irq.c
>> index cdf86cd3fd97..bc992b7b041f 100644
>> --- a/arch/x86/platform/uv/uv_irq.c
>> +++ b/arch/x86/platform/uv/uv_irq.c
>> @@ -89,7 +89,7 @@ static int uv_domain_alloc(struct irq_domain *domain,
>> unsigned int virq,
>> return -EINVAL;
>>
>> chip_data = kmalloc_node(sizeof(*chip_data), GFP_KERNEL,
>> - irq_data->node);
>> + irq_data_get_node(irq_data));
>> if (!chip_data)
>> return -ENOMEM;
>>
>> diff --git a/include/linux/irq.h b/include/linux/irq.h
>> index 3b6e0def7f5c..3f999a0af713 100644
>> --- a/include/linux/irq.h
>> +++ b/include/linux/irq.h
>> @@ -129,9 +129,13 @@ struct irq_domain;
>> * struct irq_common_data - per irq data shared by all irqchips
>> * @state_use_accessors: status information for irq chip functions.
>> * Use accessor functions to deal with it
>> + * @node: node index useful for balancing
>> */
>> struct irq_common_data {
>> unsigned intstate_use_accessors;
>> +#ifdef CONFIG_SMP
>
> Would CONFIG_NUMA be a little more appropriate?
> Or even let @node be always com
Re: [RFC v1 01/11] genirq: Introduce struct irq_common_data to host shared irq data
On 2015/5/4 11:15, Jiang Liu wrote:
[...]
> diff --git a/include/linux/irqdesc.h b/include/linux/irqdesc.h
> index dd1109fb241e..3010e99abf3e 100644
> --- a/include/linux/irqdesc.h
> +++ b/include/linux/irqdesc.h
> @@ -47,6 +47,7 @@ struct pt_regs;
> * @name:flow handler name for /proc/interrupts output
> */
> struct irq_desc {
> + struct irq_common_data irq_common_data;
Hi Gerry,
Please update description as well. :)
Thanks,
Abel
> struct irq_data irq_data;
> unsigned int __percpu *kstat_irqs;
> irq_flow_handler_t handle_irq;
> diff --git a/kernel/irq/internals.h b/kernel/irq/internals.h
> index df553b0af936..ed84299788b3 100644
> --- a/kernel/irq/internals.h
> +++ b/kernel/irq/internals.h
> @@ -170,27 +170,27 @@ irq_put_desc_unlock(struct irq_desc *desc, unsigned
> long flags)
> */
> static inline void irqd_set_move_pending(struct irq_data *d)
> {
> - d->state_use_accessors |= IRQD_SETAFFINITY_PENDING;
> + __irqd_to_state(d) |= IRQD_SETAFFINITY_PENDING;
> }
>
> static inline void irqd_clr_move_pending(struct irq_data *d)
> {
> - d->state_use_accessors &= ~IRQD_SETAFFINITY_PENDING;
> + __irqd_to_state(d) &= ~IRQD_SETAFFINITY_PENDING;
> }
>
> static inline void irqd_clear(struct irq_data *d, unsigned int mask)
> {
> - d->state_use_accessors &= ~mask;
> + __irqd_to_state(d) &= ~mask;
> }
>
> static inline void irqd_set(struct irq_data *d, unsigned int mask)
> {
> - d->state_use_accessors |= mask;
> + __irqd_to_state(d) |= mask;
> }
>
> static inline bool irqd_has_set(struct irq_data *d, unsigned int mask)
> {
> - return d->state_use_accessors & mask;
> + return __irqd_to_state(d) & mask;
> }
>
> static inline void kstat_incr_irqs_this_cpu(unsigned int irq, struct
> irq_desc *desc)
> diff --git a/kernel/irq/irqdesc.c b/kernel/irq/irqdesc.c
> index 99793b9b6d23..eac1aac906ea 100644
> --- a/kernel/irq/irqdesc.c
> +++ b/kernel/irq/irqdesc.c
> @@ -76,6 +76,7 @@ static void desc_set_defaults(unsigned int irq, struct
> irq_desc *desc, int node,
> {
> int cpu;
>
> + desc->irq_data.common = >irq_common_data;
> desc->irq_data.irq = irq;
> desc->irq_data.chip = _irq_chip;
> desc->irq_data.chip_data = NULL;
> diff --git a/kernel/irq/irqdomain.c b/kernel/irq/irqdomain.c
> index 7fac311057b8..3552b8750efd 100644
> --- a/kernel/irq/irqdomain.c
> +++ b/kernel/irq/irqdomain.c
> @@ -834,6 +834,7 @@ static struct irq_data *irq_domain_insert_irq_data(struct
> irq_domain *domain,
> if (irq_data) {
> child->parent_data = irq_data;
> irq_data->irq = child->irq;
> + irq_data->common = child->common;
> irq_data->node = child->node;
> irq_data->domain = domain;
> }
--
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: [RFC v1 02/11] genirq: Move field 'node' from struct irq_data into struct irq_common_data
Hi Gerry,
On 2015/5/4 11:15, Jiang Liu wrote:
> NUMA node information is per-irq instead of per-irqchip, so move it into
> struct irq_common_data.
>
> Signed-off-by: Jiang Liu
> ---
> arch/sh/kernel/irq.c |2 +-
> arch/x86/kernel/apic/vector.c |8
> arch/x86/platform/uv/uv_irq.c |2 +-
> include/linux/irq.h | 20 ++--
> kernel/irq/internals.h|5 +
> kernel/irq/irqdesc.c | 10 ++
> kernel/irq/irqdomain.c|4 ++--
> kernel/irq/manage.c |2 +-
> kernel/irq/proc.c |2 +-
> 9 files changed, 35 insertions(+), 20 deletions(-)
>
> diff --git a/arch/sh/kernel/irq.c b/arch/sh/kernel/irq.c
> index eb10ff84015c..8dc677cc136b 100644
> --- a/arch/sh/kernel/irq.c
> +++ b/arch/sh/kernel/irq.c
> @@ -227,7 +227,7 @@ void migrate_irqs(void)
> for_each_active_irq(irq) {
> struct irq_data *data = irq_get_irq_data(irq);
>
> - if (data->node == cpu) {
> + if (irq_data_get_node(data) == cpu) {
> unsigned int newcpu = cpumask_any_and(data->affinity,
> cpu_online_mask);
> if (newcpu >= nr_cpu_ids) {
> diff --git a/arch/x86/kernel/apic/vector.c b/arch/x86/kernel/apic/vector.c
> index 96ce5068a926..983bea2a09ce 100644
> --- a/arch/x86/kernel/apic/vector.c
> +++ b/arch/x86/kernel/apic/vector.c
> @@ -345,7 +345,7 @@ static int x86_vector_alloc_irqs(struct irq_domain
> *domain, unsigned int virq,
> struct irq_alloc_info *info = arg;
> struct apic_chip_data *data;
> struct irq_data *irq_data;
> - int i, err;
> + int i, err, node;
>
> if (disable_apic)
> return -ENXIO;
> @@ -357,12 +357,13 @@ static int x86_vector_alloc_irqs(struct irq_domain
> *domain, unsigned int virq,
> for (i = 0; i < nr_irqs; i++) {
> irq_data = irq_domain_get_irq_data(domain, virq + i);
> BUG_ON(!irq_data);
> + node = irq_data_get_node(irq_data);
> #ifdef CONFIG_X86_IO_APIC
> if (virq + i < nr_legacy_irqs() && legacy_irq_data[virq + i])
> data = legacy_irq_data[virq + i];
> else
> #endif
> - data = alloc_apic_chip_data(irq_data->node);
> + data = alloc_apic_chip_data(node);
> if (!data) {
> err = -ENOMEM;
> goto error;
> @@ -371,8 +372,7 @@ static int x86_vector_alloc_irqs(struct irq_domain
> *domain, unsigned int virq,
> irq_data->chip = _controller;
> irq_data->chip_data = data;
> irq_data->hwirq = virq + i;
> - err = assign_irq_vector_policy(virq, irq_data->node, data,
> -info);
> + err = assign_irq_vector_policy(virq, node, data, info);
> if (err)
> goto error;
> }
> diff --git a/arch/x86/platform/uv/uv_irq.c b/arch/x86/platform/uv/uv_irq.c
> index cdf86cd3fd97..bc992b7b041f 100644
> --- a/arch/x86/platform/uv/uv_irq.c
> +++ b/arch/x86/platform/uv/uv_irq.c
> @@ -89,7 +89,7 @@ static int uv_domain_alloc(struct irq_domain *domain,
> unsigned int virq,
> return -EINVAL;
>
> chip_data = kmalloc_node(sizeof(*chip_data), GFP_KERNEL,
> - irq_data->node);
> + irq_data_get_node(irq_data));
> if (!chip_data)
> return -ENOMEM;
>
> diff --git a/include/linux/irq.h b/include/linux/irq.h
> index 3b6e0def7f5c..3f999a0af713 100644
> --- a/include/linux/irq.h
> +++ b/include/linux/irq.h
> @@ -129,9 +129,13 @@ struct irq_domain;
> * struct irq_common_data - per irq data shared by all irqchips
> * @state_use_accessors: status information for irq chip functions.
> * Use accessor functions to deal with it
> + * @node:node index useful for balancing
> */
> struct irq_common_data {
> unsigned intstate_use_accessors;
> +#ifdef CONFIG_SMP
Would CONFIG_NUMA be a little more appropriate?
Or even let @node be always compiled?
Thanks,
Abel
> + unsigned intnode;
> +#endif
> };
>
> /**
> @@ -139,7 +143,6 @@ struct irq_common_data {
> * @mask:precomputed bitmask for accessing the chip registers
> * @irq: interrupt number
> * @hwirq: hardware interrupt number, local to the interrupt domain
> - * @node:node index useful for balancing
> * @common: point to data shared by all irqchips
> * @chip:low level interrupt hardware access
> * @domain: Interrupt translation domain; responsible for mapping
> @@ -160,7 +163,6 @@ struct irq_data {
> u32 mask;
> unsigned intirq;
> unsigned long
Re: [RFC v1 02/11] genirq: Move field 'node' from struct irq_data into struct irq_common_data
Hi Gerry,
On 2015/5/4 11:15, Jiang Liu wrote:
NUMA node information is per-irq instead of per-irqchip, so move it into
struct irq_common_data.
Signed-off-by: Jiang Liu jiang@linux.intel.com
---
arch/sh/kernel/irq.c |2 +-
arch/x86/kernel/apic/vector.c |8
arch/x86/platform/uv/uv_irq.c |2 +-
include/linux/irq.h | 20 ++--
kernel/irq/internals.h|5 +
kernel/irq/irqdesc.c | 10 ++
kernel/irq/irqdomain.c|4 ++--
kernel/irq/manage.c |2 +-
kernel/irq/proc.c |2 +-
9 files changed, 35 insertions(+), 20 deletions(-)
diff --git a/arch/sh/kernel/irq.c b/arch/sh/kernel/irq.c
index eb10ff84015c..8dc677cc136b 100644
--- a/arch/sh/kernel/irq.c
+++ b/arch/sh/kernel/irq.c
@@ -227,7 +227,7 @@ void migrate_irqs(void)
for_each_active_irq(irq) {
struct irq_data *data = irq_get_irq_data(irq);
- if (data-node == cpu) {
+ if (irq_data_get_node(data) == cpu) {
unsigned int newcpu = cpumask_any_and(data-affinity,
cpu_online_mask);
if (newcpu = nr_cpu_ids) {
diff --git a/arch/x86/kernel/apic/vector.c b/arch/x86/kernel/apic/vector.c
index 96ce5068a926..983bea2a09ce 100644
--- a/arch/x86/kernel/apic/vector.c
+++ b/arch/x86/kernel/apic/vector.c
@@ -345,7 +345,7 @@ static int x86_vector_alloc_irqs(struct irq_domain
*domain, unsigned int virq,
struct irq_alloc_info *info = arg;
struct apic_chip_data *data;
struct irq_data *irq_data;
- int i, err;
+ int i, err, node;
if (disable_apic)
return -ENXIO;
@@ -357,12 +357,13 @@ static int x86_vector_alloc_irqs(struct irq_domain
*domain, unsigned int virq,
for (i = 0; i nr_irqs; i++) {
irq_data = irq_domain_get_irq_data(domain, virq + i);
BUG_ON(!irq_data);
+ node = irq_data_get_node(irq_data);
#ifdef CONFIG_X86_IO_APIC
if (virq + i nr_legacy_irqs() legacy_irq_data[virq + i])
data = legacy_irq_data[virq + i];
else
#endif
- data = alloc_apic_chip_data(irq_data-node);
+ data = alloc_apic_chip_data(node);
if (!data) {
err = -ENOMEM;
goto error;
@@ -371,8 +372,7 @@ static int x86_vector_alloc_irqs(struct irq_domain
*domain, unsigned int virq,
irq_data-chip = lapic_controller;
irq_data-chip_data = data;
irq_data-hwirq = virq + i;
- err = assign_irq_vector_policy(virq, irq_data-node, data,
-info);
+ err = assign_irq_vector_policy(virq, node, data, info);
if (err)
goto error;
}
diff --git a/arch/x86/platform/uv/uv_irq.c b/arch/x86/platform/uv/uv_irq.c
index cdf86cd3fd97..bc992b7b041f 100644
--- a/arch/x86/platform/uv/uv_irq.c
+++ b/arch/x86/platform/uv/uv_irq.c
@@ -89,7 +89,7 @@ static int uv_domain_alloc(struct irq_domain *domain,
unsigned int virq,
return -EINVAL;
chip_data = kmalloc_node(sizeof(*chip_data), GFP_KERNEL,
- irq_data-node);
+ irq_data_get_node(irq_data));
if (!chip_data)
return -ENOMEM;
diff --git a/include/linux/irq.h b/include/linux/irq.h
index 3b6e0def7f5c..3f999a0af713 100644
--- a/include/linux/irq.h
+++ b/include/linux/irq.h
@@ -129,9 +129,13 @@ struct irq_domain;
* struct irq_common_data - per irq data shared by all irqchips
* @state_use_accessors: status information for irq chip functions.
* Use accessor functions to deal with it
+ * @node:node index useful for balancing
*/
struct irq_common_data {
unsigned intstate_use_accessors;
+#ifdef CONFIG_SMP
Would CONFIG_NUMA be a little more appropriate?
Or even let @node be always compiled?
Thanks,
Abel
+ unsigned intnode;
+#endif
};
/**
@@ -139,7 +143,6 @@ struct irq_common_data {
* @mask:precomputed bitmask for accessing the chip registers
* @irq: interrupt number
* @hwirq: hardware interrupt number, local to the interrupt domain
- * @node:node index useful for balancing
* @common: point to data shared by all irqchips
* @chip:low level interrupt hardware access
* @domain: Interrupt translation domain; responsible for mapping
@@ -160,7 +163,6 @@ struct irq_data {
u32 mask;
unsigned intirq;
unsigned long hwirq;
- unsigned intnode;
struct irq_common_data *common;
struct
Re: [RFC v1 01/11] genirq: Introduce struct irq_common_data to host shared irq data
On 2015/5/4 11:15, Jiang Liu wrote:
[...]
diff --git a/include/linux/irqdesc.h b/include/linux/irqdesc.h
index dd1109fb241e..3010e99abf3e 100644
--- a/include/linux/irqdesc.h
+++ b/include/linux/irqdesc.h
@@ -47,6 +47,7 @@ struct pt_regs;
* @name:flow handler name for /proc/interrupts output
*/
struct irq_desc {
+ struct irq_common_data irq_common_data;
Hi Gerry,
Please update description as well. :)
Thanks,
Abel
struct irq_data irq_data;
unsigned int __percpu *kstat_irqs;
irq_flow_handler_t handle_irq;
diff --git a/kernel/irq/internals.h b/kernel/irq/internals.h
index df553b0af936..ed84299788b3 100644
--- a/kernel/irq/internals.h
+++ b/kernel/irq/internals.h
@@ -170,27 +170,27 @@ irq_put_desc_unlock(struct irq_desc *desc, unsigned
long flags)
*/
static inline void irqd_set_move_pending(struct irq_data *d)
{
- d-state_use_accessors |= IRQD_SETAFFINITY_PENDING;
+ __irqd_to_state(d) |= IRQD_SETAFFINITY_PENDING;
}
static inline void irqd_clr_move_pending(struct irq_data *d)
{
- d-state_use_accessors = ~IRQD_SETAFFINITY_PENDING;
+ __irqd_to_state(d) = ~IRQD_SETAFFINITY_PENDING;
}
static inline void irqd_clear(struct irq_data *d, unsigned int mask)
{
- d-state_use_accessors = ~mask;
+ __irqd_to_state(d) = ~mask;
}
static inline void irqd_set(struct irq_data *d, unsigned int mask)
{
- d-state_use_accessors |= mask;
+ __irqd_to_state(d) |= mask;
}
static inline bool irqd_has_set(struct irq_data *d, unsigned int mask)
{
- return d-state_use_accessors mask;
+ return __irqd_to_state(d) mask;
}
static inline void kstat_incr_irqs_this_cpu(unsigned int irq, struct
irq_desc *desc)
diff --git a/kernel/irq/irqdesc.c b/kernel/irq/irqdesc.c
index 99793b9b6d23..eac1aac906ea 100644
--- a/kernel/irq/irqdesc.c
+++ b/kernel/irq/irqdesc.c
@@ -76,6 +76,7 @@ static void desc_set_defaults(unsigned int irq, struct
irq_desc *desc, int node,
{
int cpu;
+ desc-irq_data.common = desc-irq_common_data;
desc-irq_data.irq = irq;
desc-irq_data.chip = no_irq_chip;
desc-irq_data.chip_data = NULL;
diff --git a/kernel/irq/irqdomain.c b/kernel/irq/irqdomain.c
index 7fac311057b8..3552b8750efd 100644
--- a/kernel/irq/irqdomain.c
+++ b/kernel/irq/irqdomain.c
@@ -834,6 +834,7 @@ static struct irq_data *irq_domain_insert_irq_data(struct
irq_domain *domain,
if (irq_data) {
child-parent_data = irq_data;
irq_data-irq = child-irq;
+ irq_data-common = child-common;
irq_data-node = child-node;
irq_data-domain = domain;
}
--
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: [RFC v1 02/11] genirq: Move field 'node' from struct irq_data into struct irq_common_data
On 2015/5/8 10:29, Yun Wu (Abel) wrote:
Hi Gerry,
On 2015/5/4 11:15, Jiang Liu wrote:
NUMA node information is per-irq instead of per-irqchip, so move it into
struct irq_common_data.
Signed-off-by: Jiang Liu jiang@linux.intel.com
---
arch/sh/kernel/irq.c |2 +-
arch/x86/kernel/apic/vector.c |8
arch/x86/platform/uv/uv_irq.c |2 +-
include/linux/irq.h | 20 ++--
kernel/irq/internals.h|5 +
kernel/irq/irqdesc.c | 10 ++
kernel/irq/irqdomain.c|4 ++--
kernel/irq/manage.c |2 +-
kernel/irq/proc.c |2 +-
9 files changed, 35 insertions(+), 20 deletions(-)
diff --git a/arch/sh/kernel/irq.c b/arch/sh/kernel/irq.c
index eb10ff84015c..8dc677cc136b 100644
--- a/arch/sh/kernel/irq.c
+++ b/arch/sh/kernel/irq.c
@@ -227,7 +227,7 @@ void migrate_irqs(void)
for_each_active_irq(irq) {
struct irq_data *data = irq_get_irq_data(irq);
-if (data-node == cpu) {
+if (irq_data_get_node(data) == cpu) {
unsigned int newcpu = cpumask_any_and(data-affinity,
cpu_online_mask);
if (newcpu = nr_cpu_ids) {
diff --git a/arch/x86/kernel/apic/vector.c b/arch/x86/kernel/apic/vector.c
index 96ce5068a926..983bea2a09ce 100644
--- a/arch/x86/kernel/apic/vector.c
+++ b/arch/x86/kernel/apic/vector.c
@@ -345,7 +345,7 @@ static int x86_vector_alloc_irqs(struct irq_domain
*domain, unsigned int virq,
struct irq_alloc_info *info = arg;
struct apic_chip_data *data;
struct irq_data *irq_data;
-int i, err;
+int i, err, node;
if (disable_apic)
return -ENXIO;
@@ -357,12 +357,13 @@ static int x86_vector_alloc_irqs(struct irq_domain
*domain, unsigned int virq,
for (i = 0; i nr_irqs; i++) {
irq_data = irq_domain_get_irq_data(domain, virq + i);
BUG_ON(!irq_data);
+node = irq_data_get_node(irq_data);
#ifdef CONFIG_X86_IO_APIC
if (virq + i nr_legacy_irqs() legacy_irq_data[virq + i])
data = legacy_irq_data[virq + i];
else
#endif
-data = alloc_apic_chip_data(irq_data-node);
+data = alloc_apic_chip_data(node);
if (!data) {
err = -ENOMEM;
goto error;
@@ -371,8 +372,7 @@ static int x86_vector_alloc_irqs(struct irq_domain
*domain, unsigned int virq,
irq_data-chip = lapic_controller;
irq_data-chip_data = data;
irq_data-hwirq = virq + i;
-err = assign_irq_vector_policy(virq, irq_data-node, data,
- info);
+err = assign_irq_vector_policy(virq, node, data, info);
if (err)
goto error;
}
diff --git a/arch/x86/platform/uv/uv_irq.c b/arch/x86/platform/uv/uv_irq.c
index cdf86cd3fd97..bc992b7b041f 100644
--- a/arch/x86/platform/uv/uv_irq.c
+++ b/arch/x86/platform/uv/uv_irq.c
@@ -89,7 +89,7 @@ static int uv_domain_alloc(struct irq_domain *domain,
unsigned int virq,
return -EINVAL;
chip_data = kmalloc_node(sizeof(*chip_data), GFP_KERNEL,
- irq_data-node);
+ irq_data_get_node(irq_data));
if (!chip_data)
return -ENOMEM;
diff --git a/include/linux/irq.h b/include/linux/irq.h
index 3b6e0def7f5c..3f999a0af713 100644
--- a/include/linux/irq.h
+++ b/include/linux/irq.h
@@ -129,9 +129,13 @@ struct irq_domain;
* struct irq_common_data - per irq data shared by all irqchips
* @state_use_accessors: status information for irq chip functions.
* Use accessor functions to deal with it
+ * @node: node index useful for balancing
*/
struct irq_common_data {
unsigned intstate_use_accessors;
+#ifdef CONFIG_SMP
Would CONFIG_NUMA be a little more appropriate?
Or even let @node be always compiled?
Sorry for comment before reading your next patch in which you replaced
CONFIG_SMP with CONFIG_NUMA.
And letting @node be under CONFIG_NUMA makes sense, because it's useless
in non-NUMA scenario.
Thanks,
Abel
+unsigned intnode;
+#endif
};
/**
@@ -139,7 +143,6 @@ struct irq_common_data {
* @mask: precomputed bitmask for accessing the chip registers
* @irq:interrupt number
* @hwirq: hardware interrupt number, local to the interrupt domain
- * @node: node index useful for balancing
* @common: point to data shared by all irqchips
* @chip: low level interrupt hardware access
* @domain: Interrupt translation domain; responsible for mapping
@@ -160,7 +163,6 @@ struct irq_data {
u32
Re: [PATCH v3 0/5] enhance configuring an ITS
On 2015/3/5 20:12, Marc Zyngier wrote: > On 04/03/15 03:18, Yun Wu wrote: >> This patch series makes some enhancement to ITS configuration in the >> following aspects: >> >> o make allocation of the ITS tables more sensible >> o replace magic numbers with sensible macros >> o guarantees a safe quiescent status before initializing an ITS >> >> This patch series is based on Marc's branch[1], and tested on Hisilion >> ARM64 board with GICv3 ITS hardware. > > So this now looks pretty good (assuming you fix the couple of nits I > mentioned). > > As this relies on my branch, shall I take it and ask Jason to look at > the branch as a whole? > Yes, please. Thanks, Abel -- 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 v3 5/5] irqchip: gicv3-its: support safe initialization
On 2015/3/5 20:05, Marc Zyngier wrote:
> On 04/03/15 03:18, Yun Wu wrote:
>> It's unsafe to change the configurations of an activated ITS directly
>> since this will lead to unpredictable results. This patch guarantees
>> the ITSes being initialized are quiescent.
>>
>> Signed-off-by: Yun Wu
>> ---
>> drivers/irqchip/irq-gic-v3-its.c | 35 +++
>> 1 file changed, 35 insertions(+)
>>
>> diff --git a/drivers/irqchip/irq-gic-v3-its.c
>> b/drivers/irqchip/irq-gic-v3-its.c
>> index d13c24e..9e09aa0 100644
>> --- a/drivers/irqchip/irq-gic-v3-its.c
>> +++ b/drivers/irqchip/irq-gic-v3-its.c
>> @@ -1320,6 +1320,34 @@ static const struct irq_domain_ops its_domain_ops = {
>> .deactivate = its_irq_domain_deactivate,
>> };
>>
>> +static int its_check_quiesced(void __iomem *base)
>
> Another nitpick: Rather than "its_check_quiesced", how about
> "its_force_quiescent" instead? Because this does a lot more than just
> checking.
Yes, indeed.
>
>> +{
>> +u32 count = 100;/* 1s */
>> +u32 val;
>> +
>> +val = readl_relaxed(base + GITS_CTLR);
>> +if (val & GITS_CTLR_QUIESCENT)
>> +return 0;
>> +
>> +/* Disable the generation of all interrupts to this ITS */
>> +val &= ~GITS_CTLR_ENABLE;
>> +writel_relaxed(val, base + GITS_CTLR);
>> +
>> +/* Poll GITS_CTLR and wait until ITS becomes quiescent */
>> +while (1) {
>> +val = readl_relaxed(base + GITS_CTLR);
>> +if (val & GITS_CTLR_QUIESCENT)
>> +return 0;
>> +
>> +count--;
>> +if (!count)
>> +return -EBUSY;
>> +
>> +cpu_relax();
>> +udelay(1);
>> +}
>> +}
>> +
>
> I still dislike this repeated pattern, but I don't have a good solution
> so far.
Me too.
>
>> static int its_probe(struct device_node *node, struct irq_domain *parent)
>> {
>> struct resource res;
>> @@ -1348,6 +1376,13 @@ static int its_probe(struct device_node *node, struct
>> irq_domain *parent)
>> goto out_unmap;
>> }
>>
>> +err = its_check_quiesced(its_base);
>> +if (err) {
>> +pr_warn("%s: failed to quiesce, giving up\n",
>> +node->full_name);
>> +goto out_unmap;
>> +}
>> +
>> pr_info("ITS: %s\n", node->full_name);
>>
>> its = kzalloc(sizeof(*its), GFP_KERNEL);
>> --
>> 1.8.0
>>
>>
>>
>
> Assuming you fix the above nitpick:
>
> Acked-by: Marc Zyngier
>
Thanks,
Abel
--
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 v3 0/5] enhance configuring an ITS
On 2015/3/5 20:12, Marc Zyngier wrote: On 04/03/15 03:18, Yun Wu wrote: This patch series makes some enhancement to ITS configuration in the following aspects: o make allocation of the ITS tables more sensible o replace magic numbers with sensible macros o guarantees a safe quiescent status before initializing an ITS This patch series is based on Marc's branch[1], and tested on Hisilion ARM64 board with GICv3 ITS hardware. So this now looks pretty good (assuming you fix the couple of nits I mentioned). As this relies on my branch, shall I take it and ask Jason to look at the branch as a whole? Yes, please. Thanks, Abel -- 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 v3 5/5] irqchip: gicv3-its: support safe initialization
On 2015/3/5 20:05, Marc Zyngier wrote:
On 04/03/15 03:18, Yun Wu wrote:
It's unsafe to change the configurations of an activated ITS directly
since this will lead to unpredictable results. This patch guarantees
the ITSes being initialized are quiescent.
Signed-off-by: Yun Wu wuyun...@huawei.com
---
drivers/irqchip/irq-gic-v3-its.c | 35 +++
1 file changed, 35 insertions(+)
diff --git a/drivers/irqchip/irq-gic-v3-its.c
b/drivers/irqchip/irq-gic-v3-its.c
index d13c24e..9e09aa0 100644
--- a/drivers/irqchip/irq-gic-v3-its.c
+++ b/drivers/irqchip/irq-gic-v3-its.c
@@ -1320,6 +1320,34 @@ static const struct irq_domain_ops its_domain_ops = {
.deactivate = its_irq_domain_deactivate,
};
+static int its_check_quiesced(void __iomem *base)
Another nitpick: Rather than its_check_quiesced, how about
its_force_quiescent instead? Because this does a lot more than just
checking.
Yes, indeed.
+{
+u32 count = 100;/* 1s */
+u32 val;
+
+val = readl_relaxed(base + GITS_CTLR);
+if (val GITS_CTLR_QUIESCENT)
+return 0;
+
+/* Disable the generation of all interrupts to this ITS */
+val = ~GITS_CTLR_ENABLE;
+writel_relaxed(val, base + GITS_CTLR);
+
+/* Poll GITS_CTLR and wait until ITS becomes quiescent */
+while (1) {
+val = readl_relaxed(base + GITS_CTLR);
+if (val GITS_CTLR_QUIESCENT)
+return 0;
+
+count--;
+if (!count)
+return -EBUSY;
+
+cpu_relax();
+udelay(1);
+}
+}
+
I still dislike this repeated pattern, but I don't have a good solution
so far.
Me too.
static int its_probe(struct device_node *node, struct irq_domain *parent)
{
struct resource res;
@@ -1348,6 +1376,13 @@ static int its_probe(struct device_node *node, struct
irq_domain *parent)
goto out_unmap;
}
+err = its_check_quiesced(its_base);
+if (err) {
+pr_warn(%s: failed to quiesce, giving up\n,
+node-full_name);
+goto out_unmap;
+}
+
pr_info(ITS: %s\n, node-full_name);
its = kzalloc(sizeof(*its), GFP_KERNEL);
--
1.8.0
Assuming you fix the above nitpick:
Acked-by: Marc Zyngier marc.zyng...@arm.com
Thanks,
Abel
--
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 5/6] irqchip: gicv3-its: add support for power down
On 2015/2/17 20:27, Yun Wu (Abel) wrote:
> On 2015/2/17 19:11, Marc Zyngier wrote:
>
>> On Tue, 17 Feb 2015 10:15:15 +0000
>> "Yun Wu (Abel)" wrote:
>>
>>> On 2015/2/17 17:29, Marc Zyngier wrote:
>>>
>>>> On Sun, 15 Feb 2015 09:32:02 +
>>>> Yun Wu wrote:
>>>>
>>>>> It's unsafe to change the configurations of an activated ITS
>>>>> directly since this will lead to unpredictable results. This patch
>>>>> guarantees a safe quiescent status before initializing an ITS.
>>>>
>>>> Please change the title of this patch to reflect what it actually
>>>> does. Nothing here is about powering down anything.
>>>
>>> My miss, I will fix this in next version.
>>>
>>>>
>>>>> Signed-off-by: Yun Wu
>>>>> ---
>>>>> drivers/irqchip/irq-gic-v3-its.c | 32
>>>>> 1 file changed, 32 insertions(+)
>>>>>
>>>>> diff --git a/drivers/irqchip/irq-gic-v3-its.c
>>>>> b/drivers/irqchip/irq-gic-v3-its.c index 42c03b2..29eb665 100644
>>>>> --- a/drivers/irqchip/irq-gic-v3-its.c
>>>>> +++ b/drivers/irqchip/irq-gic-v3-its.c
>>>>> @@ -1321,6 +1321,31 @@ static const struct irq_domain_ops
>>>>> its_domain_ops = { .deactivate=
>>>>> its_irq_domain_deactivate, };
>>>>>
>>>>> +static int its_check_quiesced(void __iomem *base)
>>>>> +{
>>>>> + u32 count = 100;/* 1s */
>>>>> + u32 val;
>>>>> +
>>>>> + val = readl_relaxed(base + GITS_CTLR);
>>>>> + if (val & GITS_CTLR_QUIESCENT)
>>>>> + return 0;
>>>>> +
>>>>> + /* Disable the generation of all interrupts to this ITS */
>>>>> + val &= ~GITS_CTLR_ENABLE;
>>>>> + writel_relaxed(val, base + GITS_CTLR);
>>>>> +
>>>>> + /* Poll GITS_CTLR and wait until ITS becomes quiescent */
>>>>> + while (count--) {
>>>>> + val = readl_relaxed(base + GITS_CTLR);
>>>>> + if (val & GITS_CTLR_QUIESCENT)
>>>>> + return 0;
>>>>> + cpu_relax();
>>>>> + udelay(1);
>>>>> + }
>>>>
>>>> You're now introducing a third variant of a 1s timeout loop. Notice
>>>> a pattern?
>>>>
>>>
>>> I am not sure I know exactly what you suggest. Do you mean I should
>>> code like below to keep the coding style same as the other 2 loops?
>>>
>>> while (1) {
>>> val = readl_relaxed(base + GITS_CTLR);
>>> if (val & GITS_CTLR_QUIESCENT)
>>> return 0;
>>>
>>> count--;
>>> if (!count)
>>> return -EBUSY;
>>>
>>> cpu_relax();
>>> udelay(1);
>>> }
>>
>> That'd be a good start. But given that this is starting to be a common
>> construct, it could probably be rewritten as:
>>
>> static int its_poll_timeout(struct its_node *its, void *data,
>> int (*fn)(struct its_node *its,
>> void *data))
>> {
>> while (1) {
>> if (!fn(its, data))
>> return 0;
>>
>> ...
>> }
>> }
>>
>> and have the call sites to provide the right utility function. We also
>> have two similar timeout loops in the main GICv3 driver, so there
>> should be room for improvement.
>>
>> Thoughts?
>>
Hi Marc,
Currently I didn't make any improvement on providing a unified utility
function to do the timeout loops, because I haven't found a proper way
yet. And to achieve this, at least three aspects I can imagine are
needed to be considered.
Refactoring the existing loop functions comes first. A prototype is
showed below.
static T1 func_prototype(T2 node, char *msg, void **args)
{
u32 count = 100;
do_something_here(node, args);
while (1) {
if (condition_satisfied(node, args))
return (T1)SUCCESS;
count--;
if (!count) {
print_err_msg(msg);
return (T1)FAI
Re: [PATCH v2 5/6] irqchip: gicv3-its: add support for power down
On 2015/2/17 20:27, Yun Wu (Abel) wrote:
On 2015/2/17 19:11, Marc Zyngier wrote:
On Tue, 17 Feb 2015 10:15:15 +
Yun Wu (Abel) wuyun...@huawei.com wrote:
On 2015/2/17 17:29, Marc Zyngier wrote:
On Sun, 15 Feb 2015 09:32:02 +
Yun Wu wuyun...@huawei.com wrote:
It's unsafe to change the configurations of an activated ITS
directly since this will lead to unpredictable results. This patch
guarantees a safe quiescent status before initializing an ITS.
Please change the title of this patch to reflect what it actually
does. Nothing here is about powering down anything.
My miss, I will fix this in next version.
Signed-off-by: Yun Wu wuyun...@huawei.com
---
drivers/irqchip/irq-gic-v3-its.c | 32
1 file changed, 32 insertions(+)
diff --git a/drivers/irqchip/irq-gic-v3-its.c
b/drivers/irqchip/irq-gic-v3-its.c index 42c03b2..29eb665 100644
--- a/drivers/irqchip/irq-gic-v3-its.c
+++ b/drivers/irqchip/irq-gic-v3-its.c
@@ -1321,6 +1321,31 @@ static const struct irq_domain_ops
its_domain_ops = { .deactivate=
its_irq_domain_deactivate, };
+static int its_check_quiesced(void __iomem *base)
+{
+ u32 count = 100;/* 1s */
+ u32 val;
+
+ val = readl_relaxed(base + GITS_CTLR);
+ if (val GITS_CTLR_QUIESCENT)
+ return 0;
+
+ /* Disable the generation of all interrupts to this ITS */
+ val = ~GITS_CTLR_ENABLE;
+ writel_relaxed(val, base + GITS_CTLR);
+
+ /* Poll GITS_CTLR and wait until ITS becomes quiescent */
+ while (count--) {
+ val = readl_relaxed(base + GITS_CTLR);
+ if (val GITS_CTLR_QUIESCENT)
+ return 0;
+ cpu_relax();
+ udelay(1);
+ }
You're now introducing a third variant of a 1s timeout loop. Notice
a pattern?
I am not sure I know exactly what you suggest. Do you mean I should
code like below to keep the coding style same as the other 2 loops?
while (1) {
val = readl_relaxed(base + GITS_CTLR);
if (val GITS_CTLR_QUIESCENT)
return 0;
count--;
if (!count)
return -EBUSY;
cpu_relax();
udelay(1);
}
That'd be a good start. But given that this is starting to be a common
construct, it could probably be rewritten as:
static int its_poll_timeout(struct its_node *its, void *data,
int (*fn)(struct its_node *its,
void *data))
{
while (1) {
if (!fn(its, data))
return 0;
...
}
}
and have the call sites to provide the right utility function. We also
have two similar timeout loops in the main GICv3 driver, so there
should be room for improvement.
Thoughts?
Hi Marc,
Currently I didn't make any improvement on providing a unified utility
function to do the timeout loops, because I haven't found a proper way
yet. And to achieve this, at least three aspects I can imagine are
needed to be considered.
Refactoring the existing loop functions comes first. A prototype is
showed below.
static T1 func_prototype(T2 node, char *msg, void **args)
{
u32 count = 100;
do_something_here(node, args);
while (1) {
if (condition_satisfied(node, args))
return (T1)SUCCESS;
count--;
if (!count) {
print_err_msg(msg);
return (T1)FAIL;
}
cpu_relax();
udelay(1);
}
}
Obviously it will make things complicated to move do_something_here() and
print_err_msg() outside of func_prototype(), because func_prototype() could
be called sereval places. But the two functions are different from each
loop function, so...
static T1 func_prototype(T2 node, char *msg, void **args)
{
u32 count = 100;
do_something_here(node, args);
while (count--) {
if (condition_satisfied(node, args))
return (T1)SUCCESS;
cpu_relax();
udelay(1);
}
print_err_msg(msg);
return (T1)FAIL;
}
Now we can unify the loop part,
static bool condition_satisfied(T2 node, void **args);
static u32 poll_timeout(T2 node, void **args,
bool (*fn)(T2, void **))
{
u32 count = 100;
while (count--) {
if (fn(node, args))
break;
cpu_relax();
udelay(1);
}
return count
Re: [PATCH v2 5/6] irqchip: gicv3-its: add support for power down
On 2015/2/17 19:11, Marc Zyngier wrote:
> On Tue, 17 Feb 2015 10:15:15 +
> "Yun Wu (Abel)" wrote:
>
>> On 2015/2/17 17:29, Marc Zyngier wrote:
>>
>>> On Sun, 15 Feb 2015 09:32:02 +
>>> Yun Wu wrote:
>>>
>>>> It's unsafe to change the configurations of an activated ITS
>>>> directly since this will lead to unpredictable results. This patch
>>>> guarantees a safe quiescent status before initializing an ITS.
>>>
>>> Please change the title of this patch to reflect what it actually
>>> does. Nothing here is about powering down anything.
>>
>> My miss, I will fix this in next version.
>>
>>>
>>>> Signed-off-by: Yun Wu
>>>> ---
>>>> drivers/irqchip/irq-gic-v3-its.c | 32
>>>> 1 file changed, 32 insertions(+)
>>>>
>>>> diff --git a/drivers/irqchip/irq-gic-v3-its.c
>>>> b/drivers/irqchip/irq-gic-v3-its.c index 42c03b2..29eb665 100644
>>>> --- a/drivers/irqchip/irq-gic-v3-its.c
>>>> +++ b/drivers/irqchip/irq-gic-v3-its.c
>>>> @@ -1321,6 +1321,31 @@ static const struct irq_domain_ops
>>>> its_domain_ops = { .deactivate =
>>>> its_irq_domain_deactivate, };
>>>>
>>>> +static int its_check_quiesced(void __iomem *base)
>>>> +{
>>>> + u32 count = 100;/* 1s */
>>>> + u32 val;
>>>> +
>>>> + val = readl_relaxed(base + GITS_CTLR);
>>>> + if (val & GITS_CTLR_QUIESCENT)
>>>> + return 0;
>>>> +
>>>> + /* Disable the generation of all interrupts to this ITS */
>>>> + val &= ~GITS_CTLR_ENABLE;
>>>> + writel_relaxed(val, base + GITS_CTLR);
>>>> +
>>>> + /* Poll GITS_CTLR and wait until ITS becomes quiescent */
>>>> + while (count--) {
>>>> + val = readl_relaxed(base + GITS_CTLR);
>>>> + if (val & GITS_CTLR_QUIESCENT)
>>>> + return 0;
>>>> + cpu_relax();
>>>> + udelay(1);
>>>> + }
>>>
>>> You're now introducing a third variant of a 1s timeout loop. Notice
>>> a pattern?
>>>
>>
>> I am not sure I know exactly what you suggest. Do you mean I should
>> code like below to keep the coding style same as the other 2 loops?
>>
>> while (1) {
>> val = readl_relaxed(base + GITS_CTLR);
>> if (val & GITS_CTLR_QUIESCENT)
>> return 0;
>>
>> count--;
>> if (!count)
>> return -EBUSY;
>>
>> cpu_relax();
>> udelay(1);
>> }
>
> That'd be a good start. But given that this is starting to be a common
> construct, it could probably be rewritten as:
>
> static int its_poll_timeout(struct its_node *its, void *data,
> int (*fn)(struct its_node *its,
> void *data))
> {
> while (1) {
> if (!fn(its, data))
> return 0;
>
> ...
> }
> }
>
> and have the call sites to provide the right utility function. We also
> have two similar timeout loops in the main GICv3 driver, so there
> should be room for improvement.
>
> Thoughts?
>
It looks fine to me. I will make some improvement in the next version after
Chinese Spring Festival. :)
Thanks,
Abel
--
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 5/6] irqchip: gicv3-its: add support for power down
On 2015/2/17 17:29, Marc Zyngier wrote:
> On Sun, 15 Feb 2015 09:32:02 +
> Yun Wu wrote:
>
>> It's unsafe to change the configurations of an activated ITS directly
>> since this will lead to unpredictable results. This patch guarantees
>> a safe quiescent status before initializing an ITS.
>
> Please change the title of this patch to reflect what it actually
> does. Nothing here is about powering down anything.
My miss, I will fix this in next version.
>
>> Signed-off-by: Yun Wu
>> ---
>> drivers/irqchip/irq-gic-v3-its.c | 32
>> 1 file changed, 32 insertions(+)
>>
>> diff --git a/drivers/irqchip/irq-gic-v3-its.c
>> b/drivers/irqchip/irq-gic-v3-its.c index 42c03b2..29eb665 100644
>> --- a/drivers/irqchip/irq-gic-v3-its.c
>> +++ b/drivers/irqchip/irq-gic-v3-its.c
>> @@ -1321,6 +1321,31 @@ static const struct irq_domain_ops
>> its_domain_ops = { .deactivate =
>> its_irq_domain_deactivate, };
>>
>> +static int its_check_quiesced(void __iomem *base)
>> +{
>> +u32 count = 100;/* 1s */
>> +u32 val;
>> +
>> +val = readl_relaxed(base + GITS_CTLR);
>> +if (val & GITS_CTLR_QUIESCENT)
>> +return 0;
>> +
>> +/* Disable the generation of all interrupts to this ITS */
>> +val &= ~GITS_CTLR_ENABLE;
>> +writel_relaxed(val, base + GITS_CTLR);
>> +
>> +/* Poll GITS_CTLR and wait until ITS becomes quiescent */
>> +while (count--) {
>> +val = readl_relaxed(base + GITS_CTLR);
>> +if (val & GITS_CTLR_QUIESCENT)
>> +return 0;
>> +cpu_relax();
>> +udelay(1);
>> +}
>
> You're now introducing a third variant of a 1s timeout loop. Notice a
> pattern?
>
I am not sure I know exactly what you suggest. Do you mean I should code
like below to keep the coding style same as the other 2 loops?
while (1) {
val = readl_relaxed(base + GITS_CTLR);
if (val & GITS_CTLR_QUIESCENT)
return 0;
count--;
if (!count)
return -EBUSY;
cpu_relax();
udelay(1);
}
Thanks,
Abel
--
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 3/6] irqchip: gicv3-its: limit order of DT size to MAX_ORDER
On 2015/2/17 17:19, Marc Zyngier wrote:
> On Sun, 15 Feb 2015 09:32:00 +
> Yun Wu wrote:
>
>> When required DT size is out of the kmalloc()'s capability, the whole
>
> Nit: Using the DT acronym is very confusing here, as it means "Device
> Tree" to most people, including me. Please use "Device Table" instead.
>
> Also, the reference to kmalloc is wrong, as we're really using the page
> allocator.
OK, I will get the description fixed in the next version.
>
>> ITS will fail in probing. This actually is not the hardware's problem
>> and is mainly a limitation of the kernel memory allocator. This patch
>> will keep ITS going on to the next initializaion step with an explicit
>> warning.
>>
>> Signed-off-by: Yun Wu
>> ---
>> drivers/irqchip/irq-gic-v3-its.c | 6 ++
>> 1 file changed, 6 insertions(+)
>>
>> diff --git a/drivers/irqchip/irq-gic-v3-its.c
>> b/drivers/irqchip/irq-gic-v3-its.c index f5bfa42..de36606 100644
>> --- a/drivers/irqchip/irq-gic-v3-its.c
>> +++ b/drivers/irqchip/irq-gic-v3-its.c
>> @@ -828,6 +828,12 @@ static int its_alloc_tables(struct its_node *its)
>> u32 ids = GITS_TYPER_DEVBITS(typer);
>>
>> order = get_order((1UL << ids) * entry_size);
>> +if (order >= MAX_ORDER) {
>> +pr_warn("%s: DT size too large,
>> reduce to %u pages\n",
>> +
>> its->msi_chip.of_node->full_name,
>> +1 << order);
>> +order = MAX_ORDER;
>> +}
>> }
>
> Something is wrong here. Either (order == MAX_ORDER) is acceptable, or
> it is not. Also, your warning says that you're reducing the size to a
> new value, but you're displaying the size you can't handle. That
> message should be fixed.
>
Yes, my fault, I will fix them.
Thanks,
Abel
--
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 5/6] irqchip: gicv3-its: add support for power down
On 2015/2/17 17:29, Marc Zyngier wrote:
On Sun, 15 Feb 2015 09:32:02 +
Yun Wu wuyun...@huawei.com wrote:
It's unsafe to change the configurations of an activated ITS directly
since this will lead to unpredictable results. This patch guarantees
a safe quiescent status before initializing an ITS.
Please change the title of this patch to reflect what it actually
does. Nothing here is about powering down anything.
My miss, I will fix this in next version.
Signed-off-by: Yun Wu wuyun...@huawei.com
---
drivers/irqchip/irq-gic-v3-its.c | 32
1 file changed, 32 insertions(+)
diff --git a/drivers/irqchip/irq-gic-v3-its.c
b/drivers/irqchip/irq-gic-v3-its.c index 42c03b2..29eb665 100644
--- a/drivers/irqchip/irq-gic-v3-its.c
+++ b/drivers/irqchip/irq-gic-v3-its.c
@@ -1321,6 +1321,31 @@ static const struct irq_domain_ops
its_domain_ops = { .deactivate =
its_irq_domain_deactivate, };
+static int its_check_quiesced(void __iomem *base)
+{
+u32 count = 100;/* 1s */
+u32 val;
+
+val = readl_relaxed(base + GITS_CTLR);
+if (val GITS_CTLR_QUIESCENT)
+return 0;
+
+/* Disable the generation of all interrupts to this ITS */
+val = ~GITS_CTLR_ENABLE;
+writel_relaxed(val, base + GITS_CTLR);
+
+/* Poll GITS_CTLR and wait until ITS becomes quiescent */
+while (count--) {
+val = readl_relaxed(base + GITS_CTLR);
+if (val GITS_CTLR_QUIESCENT)
+return 0;
+cpu_relax();
+udelay(1);
+}
You're now introducing a third variant of a 1s timeout loop. Notice a
pattern?
I am not sure I know exactly what you suggest. Do you mean I should code
like below to keep the coding style same as the other 2 loops?
while (1) {
val = readl_relaxed(base + GITS_CTLR);
if (val GITS_CTLR_QUIESCENT)
return 0;
count--;
if (!count)
return -EBUSY;
cpu_relax();
udelay(1);
}
Thanks,
Abel
--
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 3/6] irqchip: gicv3-its: limit order of DT size to MAX_ORDER
On 2015/2/17 17:19, Marc Zyngier wrote:
On Sun, 15 Feb 2015 09:32:00 +
Yun Wu wuyun...@huawei.com wrote:
When required DT size is out of the kmalloc()'s capability, the whole
Nit: Using the DT acronym is very confusing here, as it means Device
Tree to most people, including me. Please use Device Table instead.
Also, the reference to kmalloc is wrong, as we're really using the page
allocator.
OK, I will get the description fixed in the next version.
ITS will fail in probing. This actually is not the hardware's problem
and is mainly a limitation of the kernel memory allocator. This patch
will keep ITS going on to the next initializaion step with an explicit
warning.
Signed-off-by: Yun Wu wuyun...@huawei.com
---
drivers/irqchip/irq-gic-v3-its.c | 6 ++
1 file changed, 6 insertions(+)
diff --git a/drivers/irqchip/irq-gic-v3-its.c
b/drivers/irqchip/irq-gic-v3-its.c index f5bfa42..de36606 100644
--- a/drivers/irqchip/irq-gic-v3-its.c
+++ b/drivers/irqchip/irq-gic-v3-its.c
@@ -828,6 +828,12 @@ static int its_alloc_tables(struct its_node *its)
u32 ids = GITS_TYPER_DEVBITS(typer);
order = get_order((1UL ids) * entry_size);
+if (order = MAX_ORDER) {
+pr_warn(%s: DT size too large,
reduce to %u pages\n,
+
its-msi_chip.of_node-full_name,
+1 order);
+order = MAX_ORDER;
+}
}
Something is wrong here. Either (order == MAX_ORDER) is acceptable, or
it is not. Also, your warning says that you're reducing the size to a
new value, but you're displaying the size you can't handle. That
message should be fixed.
Yes, my fault, I will fix them.
Thanks,
Abel
--
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 5/6] irqchip: gicv3-its: add support for power down
On 2015/2/17 19:11, Marc Zyngier wrote:
On Tue, 17 Feb 2015 10:15:15 +
Yun Wu (Abel) wuyun...@huawei.com wrote:
On 2015/2/17 17:29, Marc Zyngier wrote:
On Sun, 15 Feb 2015 09:32:02 +
Yun Wu wuyun...@huawei.com wrote:
It's unsafe to change the configurations of an activated ITS
directly since this will lead to unpredictable results. This patch
guarantees a safe quiescent status before initializing an ITS.
Please change the title of this patch to reflect what it actually
does. Nothing here is about powering down anything.
My miss, I will fix this in next version.
Signed-off-by: Yun Wu wuyun...@huawei.com
---
drivers/irqchip/irq-gic-v3-its.c | 32
1 file changed, 32 insertions(+)
diff --git a/drivers/irqchip/irq-gic-v3-its.c
b/drivers/irqchip/irq-gic-v3-its.c index 42c03b2..29eb665 100644
--- a/drivers/irqchip/irq-gic-v3-its.c
+++ b/drivers/irqchip/irq-gic-v3-its.c
@@ -1321,6 +1321,31 @@ static const struct irq_domain_ops
its_domain_ops = { .deactivate =
its_irq_domain_deactivate, };
+static int its_check_quiesced(void __iomem *base)
+{
+ u32 count = 100;/* 1s */
+ u32 val;
+
+ val = readl_relaxed(base + GITS_CTLR);
+ if (val GITS_CTLR_QUIESCENT)
+ return 0;
+
+ /* Disable the generation of all interrupts to this ITS */
+ val = ~GITS_CTLR_ENABLE;
+ writel_relaxed(val, base + GITS_CTLR);
+
+ /* Poll GITS_CTLR and wait until ITS becomes quiescent */
+ while (count--) {
+ val = readl_relaxed(base + GITS_CTLR);
+ if (val GITS_CTLR_QUIESCENT)
+ return 0;
+ cpu_relax();
+ udelay(1);
+ }
You're now introducing a third variant of a 1s timeout loop. Notice
a pattern?
I am not sure I know exactly what you suggest. Do you mean I should
code like below to keep the coding style same as the other 2 loops?
while (1) {
val = readl_relaxed(base + GITS_CTLR);
if (val GITS_CTLR_QUIESCENT)
return 0;
count--;
if (!count)
return -EBUSY;
cpu_relax();
udelay(1);
}
That'd be a good start. But given that this is starting to be a common
construct, it could probably be rewritten as:
static int its_poll_timeout(struct its_node *its, void *data,
int (*fn)(struct its_node *its,
void *data))
{
while (1) {
if (!fn(its, data))
return 0;
...
}
}
and have the call sites to provide the right utility function. We also
have two similar timeout loops in the main GICv3 driver, so there
should be room for improvement.
Thoughts?
It looks fine to me. I will make some improvement in the next version after
Chinese Spring Festival. :)
Thanks,
Abel
--
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 6/6] irqchip: gicv3: skip ITS init when no ITS available
Hi Murzin,
On 2015/2/16 18:05, Vladimir Murzin wrote:
> Hi Yun,
>
> On 15/02/15 09:32, Yun Wu wrote:
>> There is one more condition that needs to be considered when judging
>> whether LPI feature is enabled or not, which is whether there is any
>> ITS available and correctly enabled.
>>
>> This patch will fix this by caching ITS enabling status in the GIC
>> chip data structure.
>
> I posted patch for that before [1] and it landed in Marc's tree
> (irq/gic-fixes). It is not clear from the commit message what the "one
> more condition" is, but I guess it is the same dts stuff. Do you see
> issue without your patch applied?
Oh yes, your patch perfectly solved this problem, so this one is no longer
needed. And sorry for not noticing your patch. :)
Thanks,
Abel
>
> [1]
> http://lists.infradead.org/pipermail/linux-arm-kernel/2015-January/314752.html
>
> Thanks
> Vladimir
>
>>
>> Signed-off-by: Yun Wu
>> ---
>> drivers/irqchip/irq-gic-v3.c | 18 +-
>> 1 file changed, 9 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/irqchip/irq-gic-v3.c b/drivers/irqchip/irq-gic-v3.c
>> index 1a146cc..e17faca 100644
>> --- a/drivers/irqchip/irq-gic-v3.c
>> +++ b/drivers/irqchip/irq-gic-v3.c
>> @@ -47,6 +47,7 @@ struct gic_chip_data {
>> u64 redist_stride;
>> u32 nr_redist_regions;
>> unsigned intirq_nr;
>> +int lpi_enabled;
>> };
>>
>> static struct gic_chip_data gic_data __read_mostly;
>> @@ -390,11 +391,6 @@ static void gic_cpu_sys_reg_init(void)
>> gic_write_grpen1(1);
>> }
>>
>> -static int gic_dist_supports_lpis(void)
>> -{
>> -return !!(readl_relaxed(gic_data.dist_base + GICD_TYPER) &
>> GICD_TYPER_LPIS);
>> -}
>> -
>> static void gic_cpu_init(void)
>> {
>> void __iomem *rbase;
>> @@ -410,7 +406,7 @@ static void gic_cpu_init(void)
>> gic_cpu_config(rbase, gic_redist_wait_for_rwp);
>>
>> /* Give LPIs a spin */
>> -if (IS_ENABLED(CONFIG_ARM_GIC_V3_ITS) && gic_dist_supports_lpis())
>> +if (gic_data.lpi_enabled)
>> its_cpu_init();
>>
>> /* initialise system registers */
>> @@ -629,7 +625,7 @@ static int gic_irq_domain_map(struct irq_domain *d,
>> unsigned int irq,
>> }
>> /* LPIs */
>> if (hw >= 8192 && hw < GIC_ID_NR) {
>> -if (!gic_dist_supports_lpis())
>> +if (!gic_data.lpi_enabled)
>> return -EPERM;
>> irq_domain_set_info(d, irq, hw, _chip, d->host_data,
>> handle_fasteoi_irq, NULL, NULL);
>> @@ -785,8 +781,12 @@ static int __init gic_of_init(struct device_node *node,
>> struct device_node *pare
>>
>> set_handle_irq(gic_handle_irq);
>>
>> -if (IS_ENABLED(CONFIG_ARM_GIC_V3_ITS) && gic_dist_supports_lpis())
>> -its_init(node, _data.rdists, gic_data.domain);
>> +if (IS_ENABLED(CONFIG_ARM_GIC_V3_ITS) &&
>> +!!(readl_relaxed(dist_base + GICD_TYPER) & GICD_TYPER_LPIS) &&
>> +!its_init(node, _data.rdists, gic_data.domain))
>> +gic_data.lpi_enabled = 1;
>> +else
>> +gic_data.lpi_enabled = 0;
>>
>> gic_smp_init();
>> gic_dist_init();
>> --
>> 1.8.0
>>
--
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 6/6] irqchip: gicv3: skip ITS init when no ITS available
Hi Murzin,
On 2015/2/16 18:05, Vladimir Murzin wrote:
Hi Yun,
On 15/02/15 09:32, Yun Wu wrote:
There is one more condition that needs to be considered when judging
whether LPI feature is enabled or not, which is whether there is any
ITS available and correctly enabled.
This patch will fix this by caching ITS enabling status in the GIC
chip data structure.
I posted patch for that before [1] and it landed in Marc's tree
(irq/gic-fixes). It is not clear from the commit message what the one
more condition is, but I guess it is the same dts stuff. Do you see
issue without your patch applied?
Oh yes, your patch perfectly solved this problem, so this one is no longer
needed. And sorry for not noticing your patch. :)
Thanks,
Abel
[1]
http://lists.infradead.org/pipermail/linux-arm-kernel/2015-January/314752.html
Thanks
Vladimir
Signed-off-by: Yun Wu wuyun...@huawei.com
---
drivers/irqchip/irq-gic-v3.c | 18 +-
1 file changed, 9 insertions(+), 9 deletions(-)
diff --git a/drivers/irqchip/irq-gic-v3.c b/drivers/irqchip/irq-gic-v3.c
index 1a146cc..e17faca 100644
--- a/drivers/irqchip/irq-gic-v3.c
+++ b/drivers/irqchip/irq-gic-v3.c
@@ -47,6 +47,7 @@ struct gic_chip_data {
u64 redist_stride;
u32 nr_redist_regions;
unsigned intirq_nr;
+int lpi_enabled;
};
static struct gic_chip_data gic_data __read_mostly;
@@ -390,11 +391,6 @@ static void gic_cpu_sys_reg_init(void)
gic_write_grpen1(1);
}
-static int gic_dist_supports_lpis(void)
-{
-return !!(readl_relaxed(gic_data.dist_base + GICD_TYPER)
GICD_TYPER_LPIS);
-}
-
static void gic_cpu_init(void)
{
void __iomem *rbase;
@@ -410,7 +406,7 @@ static void gic_cpu_init(void)
gic_cpu_config(rbase, gic_redist_wait_for_rwp);
/* Give LPIs a spin */
-if (IS_ENABLED(CONFIG_ARM_GIC_V3_ITS) gic_dist_supports_lpis())
+if (gic_data.lpi_enabled)
its_cpu_init();
/* initialise system registers */
@@ -629,7 +625,7 @@ static int gic_irq_domain_map(struct irq_domain *d,
unsigned int irq,
}
/* LPIs */
if (hw = 8192 hw GIC_ID_NR) {
-if (!gic_dist_supports_lpis())
+if (!gic_data.lpi_enabled)
return -EPERM;
irq_domain_set_info(d, irq, hw, gic_chip, d-host_data,
handle_fasteoi_irq, NULL, NULL);
@@ -785,8 +781,12 @@ static int __init gic_of_init(struct device_node *node,
struct device_node *pare
set_handle_irq(gic_handle_irq);
-if (IS_ENABLED(CONFIG_ARM_GIC_V3_ITS) gic_dist_supports_lpis())
-its_init(node, gic_data.rdists, gic_data.domain);
+if (IS_ENABLED(CONFIG_ARM_GIC_V3_ITS)
+!!(readl_relaxed(dist_base + GICD_TYPER) GICD_TYPER_LPIS)
+!its_init(node, gic_data.rdists, gic_data.domain))
+gic_data.lpi_enabled = 1;
+else
+gic_data.lpi_enabled = 0;
gic_smp_init();
gic_dist_init();
--
1.8.0
--
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 1/5] irqchip: gicv3-its: allocate proper size for DT
On 2015/1/31 3:10, Marc Zyngier wrote: > On 30/01/15 07:46, Yun Wu wrote: >> A hardware implementation may be designed to search the device >> table (DT) using a direct mapping between device ID and memory >> address, and in this scenario a single page, currently allocated >> for DT in ITS driver, will be probably not enough. >> >> This patch will try best to get this addressed by enlarging DT >> size with a limitation of MAX_ORDER pages. >> >> Signed-off-by: Yun Wu > > A similar patch has been posted already (and is already in my queue): > > https://git.kernel.org/cgit/linux/kernel/git/maz/arm-platforms.git/commit/?h=irq/gic-fixes=4be3de2af2a58476f84d678f3e8a3596f23f80d5 > Oh, now I see it. How about allocating a order of MAX_ORDER pages and throwing out a warning if the number of device id bits exceeds maximum order kernel supports, instead of letting the ITS fail in probing. Thanks, Abel -- 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 5/5] irqchip: gicv3-its: add support for power down
On 2015/1/31 3:23, Marc Zyngier wrote: > On 30/01/15 07:46, Yun Wu wrote: >> Configurations of an ITS cannot be changed if the ITS is in an >> active status, so it might not be safe to perform a soft reboot >> with all the active ITSes un-disabled, etc. kexec. >> >> This patch will make sure all the active ITSes disabled before >> enabling them again without resetting ITS hardware. > > And what happens if the kernel crashes or gets rebooted from a watchdog? > Or if the bootloader messes things up? The ITS is in an unknown state > when we start again. > > Wouldn't it be better to address this instead? Enforcing an safe initial > state seems a better solution that relying on mechanisms that may not be > relevant for all cases. > Sure, checking the ITS state before initializing it is really a better solution, I will rewrite this patch and test again. Thanks, Abel -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 3/5] irqchip: gicv3-its: use 64KB page as default granule
On 2015/1/31 3:18, Marc Zyngier wrote:
> On 30/01/15 07:46, Yun Wu wrote:
>> The field of page size in register GITS_BASERn might be read-only
>> if an implementation only supports a single, fixed page size. But
>> currently the ITS driver will throw out an error when PAGE_SIZE
>> is less than the minimum size supported by an ITS. So addressing
>> this problem by using 64KB pages as default granule for all the
>> ITS base tables.
>
> Do you actually know of an implementation with such a behaviour?
Yes, Hisilicon implemented a fixed page size of 16KB.
>
>> Signed-off-by: Yun Wu
>> ---
>> drivers/irqchip/irq-gic-v3-its.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/irqchip/irq-gic-v3-its.c
>> b/drivers/irqchip/irq-gic-v3-its.c
>> index 2a08d85..430bc92 100644
>> --- a/drivers/irqchip/irq-gic-v3-its.c
>> +++ b/drivers/irqchip/irq-gic-v3-its.c
>> @@ -801,7 +801,7 @@ static int its_alloc_tables(struct its_node *its)
>> int err;
>> int i;
>> int size;
>> -int psz = PAGE_SIZE;
>> +int psz = SZ_64K;
>> u64 shr = GITS_BASER_InnerShareable;
>>
>> for (i = 0; i < GITS_BASER_NR_REGS; i++) {
>
> Assuming such an implementation exists, you'll have to rebase this on
> top of the patch I mentioned in my reply to patch #1.
>
OK, I will.
Thanks,
Abel
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 3/5] irqchip: gicv3-its: use 64KB page as default granule
On 2015/1/31 3:18, Marc Zyngier wrote:
On 30/01/15 07:46, Yun Wu wrote:
The field of page size in register GITS_BASERn might be read-only
if an implementation only supports a single, fixed page size. But
currently the ITS driver will throw out an error when PAGE_SIZE
is less than the minimum size supported by an ITS. So addressing
this problem by using 64KB pages as default granule for all the
ITS base tables.
Do you actually know of an implementation with such a behaviour?
Yes, Hisilicon implemented a fixed page size of 16KB.
Signed-off-by: Yun Wu wuyun...@huawei.com
---
drivers/irqchip/irq-gic-v3-its.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/irqchip/irq-gic-v3-its.c
b/drivers/irqchip/irq-gic-v3-its.c
index 2a08d85..430bc92 100644
--- a/drivers/irqchip/irq-gic-v3-its.c
+++ b/drivers/irqchip/irq-gic-v3-its.c
@@ -801,7 +801,7 @@ static int its_alloc_tables(struct its_node *its)
int err;
int i;
int size;
-int psz = PAGE_SIZE;
+int psz = SZ_64K;
u64 shr = GITS_BASER_InnerShareable;
for (i = 0; i GITS_BASER_NR_REGS; i++) {
Assuming such an implementation exists, you'll have to rebase this on
top of the patch I mentioned in my reply to patch #1.
OK, I will.
Thanks,
Abel
--
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 1/5] irqchip: gicv3-its: allocate proper size for DT
On 2015/1/31 3:10, Marc Zyngier wrote: On 30/01/15 07:46, Yun Wu wrote: A hardware implementation may be designed to search the device table (DT) using a direct mapping between device ID and memory address, and in this scenario a single page, currently allocated for DT in ITS driver, will be probably not enough. This patch will try best to get this addressed by enlarging DT size with a limitation of MAX_ORDER pages. Signed-off-by: Yun Wu wuyun...@huawei.com A similar patch has been posted already (and is already in my queue): https://git.kernel.org/cgit/linux/kernel/git/maz/arm-platforms.git/commit/?h=irq/gic-fixesid=4be3de2af2a58476f84d678f3e8a3596f23f80d5 Oh, now I see it. How about allocating a order of MAX_ORDER pages and throwing out a warning if the number of device id bits exceeds maximum order kernel supports, instead of letting the ITS fail in probing. Thanks, Abel -- 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 5/5] irqchip: gicv3-its: add support for power down
On 2015/1/31 3:23, Marc Zyngier wrote: On 30/01/15 07:46, Yun Wu wrote: Configurations of an ITS cannot be changed if the ITS is in an active status, so it might not be safe to perform a soft reboot with all the active ITSes un-disabled, etc. kexec. This patch will make sure all the active ITSes disabled before enabling them again without resetting ITS hardware. And what happens if the kernel crashes or gets rebooted from a watchdog? Or if the bootloader messes things up? The ITS is in an unknown state when we start again. Wouldn't it be better to address this instead? Enforcing an safe initial state seems a better solution that relying on mechanisms that may not be relevant for all cases. Sure, checking the ITS state before initializing it is really a better solution, I will rewrite this patch and test again. Thanks, Abel -- 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 3/8] PCI/MSI: of: Add support for OF-provided msi_domain
On 2015/1/9 1:06, Marc Zyngier wrote: > In order to populate the PHB msi_domain, use the "msi-parent" > attribute to lookup a corresponding irq domain. If found, > this is our MSI domain. > > This gets plugged into the core PCI code. > Hi Marc, Since the whole patch series based on the fact that non PCI type buses will use MSI-like interrupts, would it be better if getting this field settled during OF populating? Regards, Abel -- 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 3/8] PCI/MSI: of: Add support for OF-provided msi_domain
On 2015/1/9 1:06, Marc Zyngier wrote: In order to populate the PHB msi_domain, use the msi-parent attribute to lookup a corresponding irq domain. If found, this is our MSI domain. This gets plugged into the core PCI code. Hi Marc, Since the whole patch series based on the fact that non PCI type buses will use MSI-like interrupts, would it be better if getting this field settled during OF populating? Regards, Abel -- 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 08/16] genirq: Introduce callback irq_chip.irq_write_msi_msg
On 2014/12/10 18:25, Thomas Gleixner wrote: > On Wed, 10 Dec 2014, Yun Wu (Abel) wrote: >> On 2014/11/19 19:11, Thomas Gleixner wrote: >> I spent last two weeks implementing and testing my original idea about making >> the sub domains generic, based on stacked domain feature. Now it comes real, >> please see the attached patch. > > Can you please send patches inline? Attached is a pain to reply to. Sure, why not. > >> With this patch applied, I think things will get easier. > > I don't see what gets easier. It's just another infrastructure which > is painfully similar to MSI. Then please help me feel that pain when I post inline patches, thanks. :) > >> This patch (also with several other patches) is tested on Hisilicon ARM64 >> SoC, >> with non PCI devices capable of message based interrupts. The PCI part is not >> tested because it needs large refactoring work to do. So yes, the testing >> work >> is not sufficient, but I think the patch is enough to present what I really >> wanted to express. :) > > Not really. > > If you provide proper patches which make use of it and most important > a proper refactoring of the PCI/MSI side then we can discuss that, but > for now it's just handwaving. > OK, I will start a new thread when I finished PCI/MSI refactoring work. Thanks, Abel -- 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 08/16] genirq: Introduce callback irq_chip.irq_write_msi_msg
On 2014/11/19 17:20, Marc Zyngier wrote: > On Wed, Nov 19 2014 at 6:57:25 am GMT, "Yun Wu (Abel)" > wrote: >> On 2014/11/18 22:32, Thomas Gleixner wrote: >> >>> On Tue, 18 Nov 2014, Yun Wu (Abel) wrote: >>> >>> Can you please trim the messages when you're replying? >>> >>>> The above you described is absolutely right, but not the things I want >>>> to know. :) >>>> Take GICv3 ITS for example, it deals with both PCI and non PCI message >>>> interrupts. IIUC, several irq_chips need to be implemented in the ITS >>>> driver (i.e. pci_msi_chip, A_msi_chip and B_msi_chip). What should we >>>> do to the ITS driver if new MSI-capable devices come out? >>> >>> You seem to miss the stacking here >>> >>> PCI-MSI -> >>> A-MSI -> ITS -> GIC >>> B-MSI -> >>> >>> So each of the device types has its own MSI controller. Each of them >>> will have their own callbacks and are backed by the underlying ITS/GIC >>> implementation. >> >> Yes, this hits the key point. Once a new device type becomes available, >> we need to add pieces of code outside the new device's driver to make >> it work, which in my opinion is due to lack of core infrastructure. >> More specifically, the core infrastructure needs to support mechanism >> of MSI, not the various types of devices. >> >>> >>> And that's the only sensible solution. >>> >> >> It's sensible, but not perfect. >> What I suggested is to add a generic layer: >> >> PCI-MSI -> >> A-MSI-> (generic layer) -> ITS -> GICR >> B-MSI-> >> >> The PCI/A/B/... passes its hardware properties to the generic layer which >> gets configurations ready by calling ITS's domain/chip callbacks. When >> a new device type arrives, the only thing need to do is to implement the >> driver of that device, with nothing to do with the generic layer or ITS. > > I really don't get your "generic layer" story. To me, it looks like a > glorified set of function pointers. And that's exactly what stacked > domains are giving you: > > A-MSI -> ITS -> GIC > > This "A-MSI" is responsible for: > - implementing the "prepare" callback, which allocates the ITT > - implementing the "irq_compose_msi_msg" > > Hardly anything to change in the ITS driver, and I can probably make it > so that you don't even have to write a single line of code in the ITS > driver. > > If the generic MSI layer we now have is not enough for you, then please > submit detailed use cases. > Hi Marc, As I said, I never thought Gerry's patch don't work, I am just trying to make it better. :) As to the "generic layer" story, please check the following URL: https://lkml.org/lkml/2014/12/10/93 Thanks, Abel -- 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 08/16] genirq: Introduce callback irq_chip.irq_write_msi_msg
On 2014/11/19 19:11, Thomas Gleixner wrote:
> On Wed, 19 Nov 2014, Yun Wu (Abel) wrote:
>> On 2014/11/19 1:21, Marc Zyngier wrote:
>>> This is why the framework gives you the opportunity to provide methods
>>> that:
>>> - compose the message
>>> - program the message into the device
>>>
>>> None of that has to be PCI specific, and gives you a clean
>>> abstraction. The framework only gives you a number of shortcuts for PCI
>>> MSI, because that's what most people care about.
>>>
>>
>> Indeed, and I never said Jiang's patches don't work, I was just thinking
>> that they were not that perfect.
>
> But your magic extra layer of indirection is perfect? It's not, it
> just violates sane layering in order to support braindead hardware
> designs.
>
Hi Thomas, Gerry and Marc,
I spent last two weeks implementing and testing my original idea about making
the sub domains generic, based on stacked domain feature. Now it comes real,
please see the attached patch.
With this patch applied, I think things will get easier.
For drivers of interrupt controllers, they need to implement:
a) struct irq_chip, gets associated in domain's map/alloc callback
b) struct irq_domain, with corresponding operations
c) create sub generic MBI domain of IRQ domain to deal with all MBI types.
This changes almost nothing of the current code.
For drivers of MBI-capable devices, they need to implement:
a) MBI operations, like mask/unmask or setting message.
This will remove current ugly arch-specific code by organizing the device
behavior into a generic structure used in generic MBI layer.
The MBI generic code will build the bridge between the two groups. So when a
new driver need to implement, either controller's or endpoint's, just code
the corresponding 'need' described above with no more work to do.
This patch (also with several other patches) is tested on Hisilicon ARM64 SoC,
with non PCI devices capable of message based interrupts. The PCI part is not
tested because it needs large refactoring work to do. So yes, the testing work
is not sufficient, but I think the patch is enough to present what I really
wanted to express. :)
A new term introduced by the patch named Message Based Interrupt (MBI) is used
for presenting the generic MSIs (which does help me avoid conflicting with the
existing code). Actually the new name is proposed by Marc several months ago,
suggesting that MSI implies too much about PCI. I think it's a good idea to use
MBI in generic code and make the MSI/MSI-x a wrapper of MBI inside the PCI core.
Anyway, naming is not the key point yet.
Finally, yes, my thoughts is not perfect, but I am just trying to make it
better.
Best regards and thanks,
Abel
>From 64c5440f685440bb7d92ab716013f9f54f21bca2 Mon Sep 17 00:00:00 2001
From: Yun Wu
Date: Wed, 10 Dec 2014 10:32:58 +0800
Subject: [PATCH] MBI: initial support for message based interrupts
This patch provides initial support for Message Based Interrupt (MBI),
which is a write from the device to a special address which causes an
interrupt to be received by the CPU.
MBI is a generic mechanism not specific to any architectures or any
subsystems. MSI/MSI-X defined in PCI specification are special MBIs.
Signed-off-by: Yun Wu
---
arch/arm64/Kconfig | 1 +
include/linux/device.h | 3 +
include/linux/irq.h| 22
include/linux/mbi.h| 95 ++
kernel/irq/Kconfig | 5 +
kernel/irq/Makefile| 1 +
kernel/irq/chip.c | 66
kernel/irq/mbi.c | 260 +
8 files changed, 453 insertions(+)
create mode 100644 include/linux/mbi.h
create mode 100644 kernel/irq/mbi.c
diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
index 1f49c288..ef55541 100644
--- a/arch/arm64/Kconfig
+++ b/arch/arm64/Kconfig
@@ -13,6 +13,7 @@ config ARM64
select ARM_GIC
select AUDIT_ARCH_COMPAT_GENERIC
select ARM_GIC_V3
+ select MBI
select ARM_GIC_V3_ITS if PCI_MSI
select BUILDTIME_EXTABLE_SORT
select CLONE_BACKWARDS
diff --git a/include/linux/device.h b/include/linux/device.h
index ce1f2160..e0618c2 100644
--- a/include/linux/device.h
+++ b/include/linux/device.h
@@ -715,6 +715,7 @@ struct acpi_dev_node {
* gone away. This should be set by the allocator of the
* device (i.e. the bus driver that discovered the device).
* @iommu_group: IOMMU group the device belongs to.
+ * @mbi: Pointer to the data of message based interrupts.
*
* @offline_disabled: If set, the device is permanently online.
* @offline: Set after successful invocation of bus type's .offline().
@@ -794,6 +795,8 @@ struct device {
void(*release)(struct device *dev);
struct iommu_group *iommu_group;
+ struct mbi_data *mbi;
+
bool
Re: [patch 08/16] genirq: Introduce callback irq_chip.irq_write_msi_msg
On 2014/11/19 19:11, Thomas Gleixner wrote:
On Wed, 19 Nov 2014, Yun Wu (Abel) wrote:
On 2014/11/19 1:21, Marc Zyngier wrote:
This is why the framework gives you the opportunity to provide methods
that:
- compose the message
- program the message into the device
None of that has to be PCI specific, and gives you a clean
abstraction. The framework only gives you a number of shortcuts for PCI
MSI, because that's what most people care about.
Indeed, and I never said Jiang's patches don't work, I was just thinking
that they were not that perfect.
But your magic extra layer of indirection is perfect? It's not, it
just violates sane layering in order to support braindead hardware
designs.
Hi Thomas, Gerry and Marc,
I spent last two weeks implementing and testing my original idea about making
the sub domains generic, based on stacked domain feature. Now it comes real,
please see the attached patch.
With this patch applied, I think things will get easier.
For drivers of interrupt controllers, they need to implement:
a) struct irq_chip, gets associated in domain's map/alloc callback
b) struct irq_domain, with corresponding operations
c) create sub generic MBI domain of IRQ domain to deal with all MBI types.
This changes almost nothing of the current code.
For drivers of MBI-capable devices, they need to implement:
a) MBI operations, like mask/unmask or setting message.
This will remove current ugly arch-specific code by organizing the device
behavior into a generic structure used in generic MBI layer.
The MBI generic code will build the bridge between the two groups. So when a
new driver need to implement, either controller's or endpoint's, just code
the corresponding 'need' described above with no more work to do.
This patch (also with several other patches) is tested on Hisilicon ARM64 SoC,
with non PCI devices capable of message based interrupts. The PCI part is not
tested because it needs large refactoring work to do. So yes, the testing work
is not sufficient, but I think the patch is enough to present what I really
wanted to express. :)
A new term introduced by the patch named Message Based Interrupt (MBI) is used
for presenting the generic MSIs (which does help me avoid conflicting with the
existing code). Actually the new name is proposed by Marc several months ago,
suggesting that MSI implies too much about PCI. I think it's a good idea to use
MBI in generic code and make the MSI/MSI-x a wrapper of MBI inside the PCI core.
Anyway, naming is not the key point yet.
Finally, yes, my thoughts is not perfect, but I am just trying to make it
better.
Best regards and thanks,
Abel
From 64c5440f685440bb7d92ab716013f9f54f21bca2 Mon Sep 17 00:00:00 2001
From: Yun Wu wuyun...@huawei.com
Date: Wed, 10 Dec 2014 10:32:58 +0800
Subject: [PATCH] MBI: initial support for message based interrupts
This patch provides initial support for Message Based Interrupt (MBI),
which is a write from the device to a special address which causes an
interrupt to be received by the CPU.
MBI is a generic mechanism not specific to any architectures or any
subsystems. MSI/MSI-X defined in PCI specification are special MBIs.
Signed-off-by: Yun Wu wuyun...@huawei.com
---
arch/arm64/Kconfig | 1 +
include/linux/device.h | 3 +
include/linux/irq.h| 22
include/linux/mbi.h| 95 ++
kernel/irq/Kconfig | 5 +
kernel/irq/Makefile| 1 +
kernel/irq/chip.c | 66
kernel/irq/mbi.c | 260 +
8 files changed, 453 insertions(+)
create mode 100644 include/linux/mbi.h
create mode 100644 kernel/irq/mbi.c
diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
index 1f49c288..ef55541 100644
--- a/arch/arm64/Kconfig
+++ b/arch/arm64/Kconfig
@@ -13,6 +13,7 @@ config ARM64
select ARM_GIC
select AUDIT_ARCH_COMPAT_GENERIC
select ARM_GIC_V3
+ select MBI
select ARM_GIC_V3_ITS if PCI_MSI
select BUILDTIME_EXTABLE_SORT
select CLONE_BACKWARDS
diff --git a/include/linux/device.h b/include/linux/device.h
index ce1f2160..e0618c2 100644
--- a/include/linux/device.h
+++ b/include/linux/device.h
@@ -715,6 +715,7 @@ struct acpi_dev_node {
* gone away. This should be set by the allocator of the
* device (i.e. the bus driver that discovered the device).
* @iommu_group: IOMMU group the device belongs to.
+ * @mbi: Pointer to the data of message based interrupts.
*
* @offline_disabled: If set, the device is permanently online.
* @offline: Set after successful invocation of bus type's .offline().
@@ -794,6 +795,8 @@ struct device {
void(*release)(struct device *dev);
struct iommu_group *iommu_group;
+ struct mbi_data *mbi;
+
booloffline_disabled:1;
booloffline:1;
};
diff --git a/include/linux/irq.h b/include/linux/irq.h
index
Re: [patch 08/16] genirq: Introduce callback irq_chip.irq_write_msi_msg
On 2014/11/19 17:20, Marc Zyngier wrote: On Wed, Nov 19 2014 at 6:57:25 am GMT, Yun Wu (Abel) wuyun...@huawei.com wrote: On 2014/11/18 22:32, Thomas Gleixner wrote: On Tue, 18 Nov 2014, Yun Wu (Abel) wrote: Can you please trim the messages when you're replying? The above you described is absolutely right, but not the things I want to know. :) Take GICv3 ITS for example, it deals with both PCI and non PCI message interrupts. IIUC, several irq_chips need to be implemented in the ITS driver (i.e. pci_msi_chip, A_msi_chip and B_msi_chip). What should we do to the ITS driver if new MSI-capable devices come out? You seem to miss the stacking here PCI-MSI - A-MSI - ITS - GIC B-MSI - So each of the device types has its own MSI controller. Each of them will have their own callbacks and are backed by the underlying ITS/GIC implementation. Yes, this hits the key point. Once a new device type becomes available, we need to add pieces of code outside the new device's driver to make it work, which in my opinion is due to lack of core infrastructure. More specifically, the core infrastructure needs to support mechanism of MSI, not the various types of devices. And that's the only sensible solution. It's sensible, but not perfect. What I suggested is to add a generic layer: PCI-MSI - A-MSI- (generic layer) - ITS - GICR B-MSI- The PCI/A/B/... passes its hardware properties to the generic layer which gets configurations ready by calling ITS's domain/chip callbacks. When a new device type arrives, the only thing need to do is to implement the driver of that device, with nothing to do with the generic layer or ITS. I really don't get your generic layer story. To me, it looks like a glorified set of function pointers. And that's exactly what stacked domains are giving you: A-MSI - ITS - GIC This A-MSI is responsible for: - implementing the prepare callback, which allocates the ITT - implementing the irq_compose_msi_msg Hardly anything to change in the ITS driver, and I can probably make it so that you don't even have to write a single line of code in the ITS driver. If the generic MSI layer we now have is not enough for you, then please submit detailed use cases. Hi Marc, As I said, I never thought Gerry's patch don't work, I am just trying to make it better. :) As to the generic layer story, please check the following URL: https://lkml.org/lkml/2014/12/10/93 Thanks, Abel -- 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 08/16] genirq: Introduce callback irq_chip.irq_write_msi_msg
On 2014/12/10 18:25, Thomas Gleixner wrote: On Wed, 10 Dec 2014, Yun Wu (Abel) wrote: On 2014/11/19 19:11, Thomas Gleixner wrote: I spent last two weeks implementing and testing my original idea about making the sub domains generic, based on stacked domain feature. Now it comes real, please see the attached patch. Can you please send patches inline? Attached is a pain to reply to. Sure, why not. With this patch applied, I think things will get easier. I don't see what gets easier. It's just another infrastructure which is painfully similar to MSI. Then please help me feel that pain when I post inline patches, thanks. :) This patch (also with several other patches) is tested on Hisilicon ARM64 SoC, with non PCI devices capable of message based interrupts. The PCI part is not tested because it needs large refactoring work to do. So yes, the testing work is not sufficient, but I think the patch is enough to present what I really wanted to express. :) Not really. If you provide proper patches which make use of it and most important a proper refactoring of the PCI/MSI side then we can discuss that, but for now it's just handwaving. OK, I will start a new thread when I finished PCI/MSI refactoring work. Thanks, Abel -- 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 v3 04/13] irqchip: GICv3: ITS command queue
On 2014/11/24 22:35, Marc Zyngier wrote:
[...]
> +static struct its_collection *its_build_mapd_cmd(struct its_cmd_block *cmd,
> + struct its_cmd_desc *desc)
> +{
> + unsigned long itt_addr;
> + u8 size = order_base_2(desc->its_mapd_cmd.dev->nr_ites);
If @nr_ites is 1, then @size becomes 0, and... (see below)
> +
> + itt_addr = virt_to_phys(desc->its_mapd_cmd.dev->itt);
> + itt_addr = ALIGN(itt_addr, ITS_ITT_ALIGN);
> +
> + its_encode_cmd(cmd, GITS_CMD_MAPD);
> + its_encode_devid(cmd, desc->its_mapd_cmd.dev->device_id);
> + its_encode_size(cmd, size - 1);
here (size - 1) becomes the value of ~0, which will exceed the maximum
supported bits of identifier.
Regards,
Abel
> + its_encode_itt(cmd, itt_addr);
> + its_encode_valid(cmd, desc->its_mapd_cmd.valid);
> +
> + its_fixup_cmd(cmd);
> +
> + return desc->its_mapd_cmd.dev->collection;
> +}
> +
> +static struct its_collection *its_build_mapc_cmd(struct its_cmd_block *cmd,
> + struct its_cmd_desc *desc)
> +{
> + its_encode_cmd(cmd, GITS_CMD_MAPC);
> + its_encode_collection(cmd, desc->its_mapc_cmd.col->col_id);
> + its_encode_target(cmd, desc->its_mapc_cmd.col->target_address);
> + its_encode_valid(cmd, desc->its_mapc_cmd.valid);
> +
> + its_fixup_cmd(cmd);
> +
> + return desc->its_mapc_cmd.col;
> +}
> +
> +static struct its_collection *its_build_mapvi_cmd(struct its_cmd_block *cmd,
> + struct its_cmd_desc *desc)
> +{
> + its_encode_cmd(cmd, GITS_CMD_MAPVI);
> + its_encode_devid(cmd, desc->its_mapvi_cmd.dev->device_id);
> + its_encode_event_id(cmd, desc->its_mapvi_cmd.event_id);
> + its_encode_phys_id(cmd, desc->its_mapvi_cmd.phys_id);
> + its_encode_collection(cmd, desc->its_mapvi_cmd.dev->collection->col_id);
> +
> + its_fixup_cmd(cmd);
> +
> + return desc->its_mapvi_cmd.dev->collection;
> +}
> +
> +static struct its_collection *its_build_movi_cmd(struct its_cmd_block *cmd,
> + struct its_cmd_desc *desc)
> +{
> + its_encode_cmd(cmd, GITS_CMD_MOVI);
> + its_encode_devid(cmd, desc->its_movi_cmd.dev->device_id);
> + its_encode_event_id(cmd, desc->its_movi_cmd.id);
> + its_encode_collection(cmd, desc->its_movi_cmd.col->col_id);
> +
> + its_fixup_cmd(cmd);
> +
> + return desc->its_movi_cmd.dev->collection;
> +}
> +
> +static struct its_collection *its_build_discard_cmd(struct its_cmd_block
> *cmd,
> + struct its_cmd_desc *desc)
> +{
> + its_encode_cmd(cmd, GITS_CMD_DISCARD);
> + its_encode_devid(cmd, desc->its_discard_cmd.dev->device_id);
> + its_encode_event_id(cmd, desc->its_discard_cmd.event_id);
> +
> + its_fixup_cmd(cmd);
> +
> + return desc->its_discard_cmd.dev->collection;
> +}
> +
> +static struct its_collection *its_build_inv_cmd(struct its_cmd_block *cmd,
> + struct its_cmd_desc *desc)
> +{
> + its_encode_cmd(cmd, GITS_CMD_INV);
> + its_encode_devid(cmd, desc->its_inv_cmd.dev->device_id);
> + its_encode_event_id(cmd, desc->its_inv_cmd.event_id);
> +
> + its_fixup_cmd(cmd);
> +
> + return desc->its_inv_cmd.dev->collection;
> +}
> +
> +static struct its_collection *its_build_invall_cmd(struct its_cmd_block *cmd,
> +struct its_cmd_desc *desc)
> +{
> + its_encode_cmd(cmd, GITS_CMD_INVALL);
> + its_encode_collection(cmd, desc->its_mapc_cmd.col->col_id);
> +
> + its_fixup_cmd(cmd);
> +
> + return NULL;
> +}
> +
> +static u64 its_cmd_ptr_to_offset(struct its_node *its,
> + struct its_cmd_block *ptr)
> +{
> + return (ptr - its->cmd_base) * sizeof(*ptr);
> +}
> +
> +static int its_queue_full(struct its_node *its)
> +{
> + int widx;
> + int ridx;
> +
> + widx = its->cmd_write - its->cmd_base;
> + ridx = readl_relaxed(its->base + GITS_CREADR) / sizeof(struct
> its_cmd_block);
> +
> + /* This is incredibly unlikely to happen, unless the ITS locks up. */
> + if (((widx + 1) % ITS_CMD_QUEUE_NR_ENTRIES) == ridx)
> + return 1;
> +
> + return 0;
> +}
> +
> +static struct its_cmd_block *its_allocate_entry(struct its_node *its)
> +{
> + struct its_cmd_block *cmd;
> + u32 count = 100;/* 1s! */
> +
> + while (its_queue_full(its)) {
> + count--;
> + if (!count) {
> + pr_err_ratelimited("ITS queue not draining\n");
> + return NULL;
> + }
> + cpu_relax();
> + udelay(1);
> + }
> +
> + cmd = its->cmd_write++;
> +
> + /* Handle queue wrapping */
> + if (its->cmd_write == (its->cmd_base + ITS_CMD_QUEUE_NR_ENTRIES))
> + its->cmd_write = its->cmd_base;
> +
> +
Re: [PATCH v3 04/13] irqchip: GICv3: ITS command queue
On 2014/11/24 22:35, Marc Zyngier wrote:
[...]
+static struct its_collection *its_build_mapd_cmd(struct its_cmd_block *cmd,
+ struct its_cmd_desc *desc)
+{
+ unsigned long itt_addr;
+ u8 size = order_base_2(desc-its_mapd_cmd.dev-nr_ites);
If @nr_ites is 1, then @size becomes 0, and... (see below)
+
+ itt_addr = virt_to_phys(desc-its_mapd_cmd.dev-itt);
+ itt_addr = ALIGN(itt_addr, ITS_ITT_ALIGN);
+
+ its_encode_cmd(cmd, GITS_CMD_MAPD);
+ its_encode_devid(cmd, desc-its_mapd_cmd.dev-device_id);
+ its_encode_size(cmd, size - 1);
here (size - 1) becomes the value of ~0, which will exceed the maximum
supported bits of identifier.
Regards,
Abel
+ its_encode_itt(cmd, itt_addr);
+ its_encode_valid(cmd, desc-its_mapd_cmd.valid);
+
+ its_fixup_cmd(cmd);
+
+ return desc-its_mapd_cmd.dev-collection;
+}
+
+static struct its_collection *its_build_mapc_cmd(struct its_cmd_block *cmd,
+ struct its_cmd_desc *desc)
+{
+ its_encode_cmd(cmd, GITS_CMD_MAPC);
+ its_encode_collection(cmd, desc-its_mapc_cmd.col-col_id);
+ its_encode_target(cmd, desc-its_mapc_cmd.col-target_address);
+ its_encode_valid(cmd, desc-its_mapc_cmd.valid);
+
+ its_fixup_cmd(cmd);
+
+ return desc-its_mapc_cmd.col;
+}
+
+static struct its_collection *its_build_mapvi_cmd(struct its_cmd_block *cmd,
+ struct its_cmd_desc *desc)
+{
+ its_encode_cmd(cmd, GITS_CMD_MAPVI);
+ its_encode_devid(cmd, desc-its_mapvi_cmd.dev-device_id);
+ its_encode_event_id(cmd, desc-its_mapvi_cmd.event_id);
+ its_encode_phys_id(cmd, desc-its_mapvi_cmd.phys_id);
+ its_encode_collection(cmd, desc-its_mapvi_cmd.dev-collection-col_id);
+
+ its_fixup_cmd(cmd);
+
+ return desc-its_mapvi_cmd.dev-collection;
+}
+
+static struct its_collection *its_build_movi_cmd(struct its_cmd_block *cmd,
+ struct its_cmd_desc *desc)
+{
+ its_encode_cmd(cmd, GITS_CMD_MOVI);
+ its_encode_devid(cmd, desc-its_movi_cmd.dev-device_id);
+ its_encode_event_id(cmd, desc-its_movi_cmd.id);
+ its_encode_collection(cmd, desc-its_movi_cmd.col-col_id);
+
+ its_fixup_cmd(cmd);
+
+ return desc-its_movi_cmd.dev-collection;
+}
+
+static struct its_collection *its_build_discard_cmd(struct its_cmd_block
*cmd,
+ struct its_cmd_desc *desc)
+{
+ its_encode_cmd(cmd, GITS_CMD_DISCARD);
+ its_encode_devid(cmd, desc-its_discard_cmd.dev-device_id);
+ its_encode_event_id(cmd, desc-its_discard_cmd.event_id);
+
+ its_fixup_cmd(cmd);
+
+ return desc-its_discard_cmd.dev-collection;
+}
+
+static struct its_collection *its_build_inv_cmd(struct its_cmd_block *cmd,
+ struct its_cmd_desc *desc)
+{
+ its_encode_cmd(cmd, GITS_CMD_INV);
+ its_encode_devid(cmd, desc-its_inv_cmd.dev-device_id);
+ its_encode_event_id(cmd, desc-its_inv_cmd.event_id);
+
+ its_fixup_cmd(cmd);
+
+ return desc-its_inv_cmd.dev-collection;
+}
+
+static struct its_collection *its_build_invall_cmd(struct its_cmd_block *cmd,
+struct its_cmd_desc *desc)
+{
+ its_encode_cmd(cmd, GITS_CMD_INVALL);
+ its_encode_collection(cmd, desc-its_mapc_cmd.col-col_id);
+
+ its_fixup_cmd(cmd);
+
+ return NULL;
+}
+
+static u64 its_cmd_ptr_to_offset(struct its_node *its,
+ struct its_cmd_block *ptr)
+{
+ return (ptr - its-cmd_base) * sizeof(*ptr);
+}
+
+static int its_queue_full(struct its_node *its)
+{
+ int widx;
+ int ridx;
+
+ widx = its-cmd_write - its-cmd_base;
+ ridx = readl_relaxed(its-base + GITS_CREADR) / sizeof(struct
its_cmd_block);
+
+ /* This is incredibly unlikely to happen, unless the ITS locks up. */
+ if (((widx + 1) % ITS_CMD_QUEUE_NR_ENTRIES) == ridx)
+ return 1;
+
+ return 0;
+}
+
+static struct its_cmd_block *its_allocate_entry(struct its_node *its)
+{
+ struct its_cmd_block *cmd;
+ u32 count = 100;/* 1s! */
+
+ while (its_queue_full(its)) {
+ count--;
+ if (!count) {
+ pr_err_ratelimited(ITS queue not draining\n);
+ return NULL;
+ }
+ cpu_relax();
+ udelay(1);
+ }
+
+ cmd = its-cmd_write++;
+
+ /* Handle queue wrapping */
+ if (its-cmd_write == (its-cmd_base + ITS_CMD_QUEUE_NR_ENTRIES))
+ its-cmd_write = its-cmd_base;
+
+ return cmd;
+}
+
+static struct its_cmd_block *its_post_commands(struct its_node *its)
+{
+ u64 wr = its_cmd_ptr_to_offset(its, its-cmd_write);
+
+ writel_relaxed(wr, its-base +
Re: [PATCH v3 09/13] irqchip: GICv3: ITS: MSI support
Hi Marc,
On 2014/11/24 22:35, Marc Zyngier wrote:
> Now, the bit of code that allow us to use the ITS as a MSI controller.
> Both MSI and MSI-X are supported.
>
> Signed-off-by: Marc Zyngier
> ---
> drivers/irqchip/irq-gic-v3-its.c | 176
> +
> include/linux/irqchip/arm-gic-v3.h | 6 ++
> 2 files changed, 182 insertions(+)
>
> diff --git a/drivers/irqchip/irq-gic-v3-its.c
> b/drivers/irqchip/irq-gic-v3-its.c
> index d687fd4..532c6df 100644
> --- a/drivers/irqchip/irq-gic-v3-its.c
> +++ b/drivers/irqchip/irq-gic-v3-its.c
> @@ -587,12 +587,47 @@ static int its_set_affinity(struct irq_data *d, const
> struct cpumask *mask_val,
> return IRQ_SET_MASK_OK_DONE;
> }
>
> +static void its_irq_compose_msi_msg(struct irq_data *d, struct msi_msg *msg)
> +{
> + struct its_device *its_dev = irq_data_get_irq_chip_data(d);
> + struct its_node *its;
> + u64 addr;
> +
> + its = its_dev->its;
> + addr = its->phys_base + GITS_TRANSLATER;
> +
> + msg->address_lo = addr & ((1UL << 32) - 1);
> + msg->address_hi = addr >> 32;
> + msg->data = its_get_event_id(d);
> +}
> +
> static struct irq_chip its_irq_chip = {
> .name = "ITS",
> .irq_mask = its_mask_irq,
> .irq_unmask = its_unmask_irq,
> .irq_eoi= its_eoi_irq,
> .irq_set_affinity = its_set_affinity,
> + .irq_compose_msi_msg= its_irq_compose_msi_msg,
> +};
> +
> +static void its_mask_msi_irq(struct irq_data *d)
> +{
> + pci_msi_mask_irq(d);
> + irq_chip_mask_parent(d);
> +}
> +
> +static void its_unmask_msi_irq(struct irq_data *d)
> +{
> + pci_msi_unmask_irq(d);
> + irq_chip_unmask_parent(d);
> +}
> +
> +static struct irq_chip its_msi_irq_chip = {
> + .name = "ITS-MSI",
> + .irq_unmask = its_unmask_msi_irq,
> + .irq_mask = its_mask_msi_irq,
> + .irq_eoi= irq_chip_eoi_parent,
> + .irq_write_msi_msg = pci_msi_domain_write_msg,
> };
>
> /*
> @@ -1055,3 +1090,144 @@ static void its_free_device(struct its_device
> *its_dev)
> kfree(its_dev->itt);
> kfree(its_dev);
> }
> +
> +static int its_alloc_device_irq(struct its_device *dev, irq_hw_number_t
> *hwirq)
> +{
> + int idx;
> +
> + idx = find_first_zero_bit(dev->lpi_map, dev->nr_lpis);
> + if (idx == dev->nr_lpis)
> + return -ENOSPC;
> +
> + *hwirq = dev->lpi_base + idx;
> + set_bit(idx, dev->lpi_map);
> +
> + /* Map the GIC irq ID to the device */
> + its_send_mapvi(dev, *hwirq, idx);
It would be better if we do hardware-level initialization in
domain.{activate,deactivate}.
> +
> + return 0;
> +}
> +
> +static int its_msi_prepare(struct irq_domain *domain, struct device *dev,
> +int nvec, msi_alloc_info_t *info)
> +{
> + struct pci_dev *pdev;
> + struct its_node *its;
> + u32 dev_id;
> + struct its_device *its_dev;
> +
> + if (!dev_is_pci(dev))
> + return -EINVAL;
> +
> + pdev = to_pci_dev(dev);
> + dev_id = PCI_DEVID(pdev->bus->number, pdev->devfn);
> + its = domain->parent->host_data;
> +
> + its_dev = its_find_device(its, dev_id);
> + if (WARN_ON(its_dev))
> + return -EINVAL;
> +
> + its_dev = its_create_device(its, dev_id, nvec);
> + if (!its_dev)
> + return -ENOMEM;
> +
> + dev_dbg(>dev, "ITT %d entries, %d bits\n", nvec, ilog2(nvec));
> +
> + info->scratchpad[0].ptr = its_dev;
> + info->scratchpad[1].ptr = dev;
> + return 0;
> +}
> +
> +static struct msi_domain_ops its_pci_msi_ops = {
> + .msi_prepare= its_msi_prepare,
> +};
> +
> +static struct msi_domain_info its_pci_msi_domain_info = {
> + .flags = (MSI_FLAG_USE_DEF_DOM_OPS | MSI_FLAG_USE_DEF_CHIP_OPS |
> +MSI_FLAG_MULTI_PCI_MSI | MSI_FLAG_PCI_MSIX),
> + .ops= _pci_msi_ops,
> + .chip = _msi_irq_chip,
> +};
> +
> +static int its_irq_gic_domain_alloc(struct irq_domain *domain,
> + unsigned int virq,
> + irq_hw_number_t hwirq)
> +{
> + struct of_phandle_args args;
> +
> + args.np = domain->parent->of_node;
> + args.args_count = 3;
> + args.args[0] = GIC_IRQ_TYPE_LPI;
> + args.args[1] = hwirq;
> + args.args[2] = IRQ_TYPE_EDGE_RISING;
> +
> + return irq_domain_alloc_irqs_parent(domain, virq, 1, );
> +}
> +
> +static int its_irq_domain_alloc(struct irq_domain *domain, unsigned int virq,
> + unsigned int nr_irqs, void *args)
> +{
> + msi_alloc_info_t *info = args;
> + struct its_device *its_dev = info->scratchpad[0].ptr;
> + irq_hw_number_t hwirq;
> + int err;
> + int i;
> +
> + for (i = 0; i < nr_irqs; i++) {
> + err = its_alloc_device_irq(its_dev, );
> + if (err)
> +
Re: [PATCH v3 09/13] irqchip: GICv3: ITS: MSI support
Hi Marc,
On 2014/11/24 22:35, Marc Zyngier wrote:
Now, the bit of code that allow us to use the ITS as a MSI controller.
Both MSI and MSI-X are supported.
Signed-off-by: Marc Zyngier marc.zyng...@arm.com
---
drivers/irqchip/irq-gic-v3-its.c | 176
+
include/linux/irqchip/arm-gic-v3.h | 6 ++
2 files changed, 182 insertions(+)
diff --git a/drivers/irqchip/irq-gic-v3-its.c
b/drivers/irqchip/irq-gic-v3-its.c
index d687fd4..532c6df 100644
--- a/drivers/irqchip/irq-gic-v3-its.c
+++ b/drivers/irqchip/irq-gic-v3-its.c
@@ -587,12 +587,47 @@ static int its_set_affinity(struct irq_data *d, const
struct cpumask *mask_val,
return IRQ_SET_MASK_OK_DONE;
}
+static void its_irq_compose_msi_msg(struct irq_data *d, struct msi_msg *msg)
+{
+ struct its_device *its_dev = irq_data_get_irq_chip_data(d);
+ struct its_node *its;
+ u64 addr;
+
+ its = its_dev-its;
+ addr = its-phys_base + GITS_TRANSLATER;
+
+ msg-address_lo = addr ((1UL 32) - 1);
+ msg-address_hi = addr 32;
+ msg-data = its_get_event_id(d);
+}
+
static struct irq_chip its_irq_chip = {
.name = ITS,
.irq_mask = its_mask_irq,
.irq_unmask = its_unmask_irq,
.irq_eoi= its_eoi_irq,
.irq_set_affinity = its_set_affinity,
+ .irq_compose_msi_msg= its_irq_compose_msi_msg,
+};
+
+static void its_mask_msi_irq(struct irq_data *d)
+{
+ pci_msi_mask_irq(d);
+ irq_chip_mask_parent(d);
+}
+
+static void its_unmask_msi_irq(struct irq_data *d)
+{
+ pci_msi_unmask_irq(d);
+ irq_chip_unmask_parent(d);
+}
+
+static struct irq_chip its_msi_irq_chip = {
+ .name = ITS-MSI,
+ .irq_unmask = its_unmask_msi_irq,
+ .irq_mask = its_mask_msi_irq,
+ .irq_eoi= irq_chip_eoi_parent,
+ .irq_write_msi_msg = pci_msi_domain_write_msg,
};
/*
@@ -1055,3 +1090,144 @@ static void its_free_device(struct its_device
*its_dev)
kfree(its_dev-itt);
kfree(its_dev);
}
+
+static int its_alloc_device_irq(struct its_device *dev, irq_hw_number_t
*hwirq)
+{
+ int idx;
+
+ idx = find_first_zero_bit(dev-lpi_map, dev-nr_lpis);
+ if (idx == dev-nr_lpis)
+ return -ENOSPC;
+
+ *hwirq = dev-lpi_base + idx;
+ set_bit(idx, dev-lpi_map);
+
+ /* Map the GIC irq ID to the device */
+ its_send_mapvi(dev, *hwirq, idx);
It would be better if we do hardware-level initialization in
domain.{activate,deactivate}.
+
+ return 0;
+}
+
+static int its_msi_prepare(struct irq_domain *domain, struct device *dev,
+int nvec, msi_alloc_info_t *info)
+{
+ struct pci_dev *pdev;
+ struct its_node *its;
+ u32 dev_id;
+ struct its_device *its_dev;
+
+ if (!dev_is_pci(dev))
+ return -EINVAL;
+
+ pdev = to_pci_dev(dev);
+ dev_id = PCI_DEVID(pdev-bus-number, pdev-devfn);
+ its = domain-parent-host_data;
+
+ its_dev = its_find_device(its, dev_id);
+ if (WARN_ON(its_dev))
+ return -EINVAL;
+
+ its_dev = its_create_device(its, dev_id, nvec);
+ if (!its_dev)
+ return -ENOMEM;
+
+ dev_dbg(pdev-dev, ITT %d entries, %d bits\n, nvec, ilog2(nvec));
+
+ info-scratchpad[0].ptr = its_dev;
+ info-scratchpad[1].ptr = dev;
+ return 0;
+}
+
+static struct msi_domain_ops its_pci_msi_ops = {
+ .msi_prepare= its_msi_prepare,
+};
+
+static struct msi_domain_info its_pci_msi_domain_info = {
+ .flags = (MSI_FLAG_USE_DEF_DOM_OPS | MSI_FLAG_USE_DEF_CHIP_OPS |
+MSI_FLAG_MULTI_PCI_MSI | MSI_FLAG_PCI_MSIX),
+ .ops= its_pci_msi_ops,
+ .chip = its_msi_irq_chip,
+};
+
+static int its_irq_gic_domain_alloc(struct irq_domain *domain,
+ unsigned int virq,
+ irq_hw_number_t hwirq)
+{
+ struct of_phandle_args args;
+
+ args.np = domain-parent-of_node;
+ args.args_count = 3;
+ args.args[0] = GIC_IRQ_TYPE_LPI;
+ args.args[1] = hwirq;
+ args.args[2] = IRQ_TYPE_EDGE_RISING;
+
+ return irq_domain_alloc_irqs_parent(domain, virq, 1, args);
+}
+
+static int its_irq_domain_alloc(struct irq_domain *domain, unsigned int virq,
+ unsigned int nr_irqs, void *args)
+{
+ msi_alloc_info_t *info = args;
+ struct its_device *its_dev = info-scratchpad[0].ptr;
+ irq_hw_number_t hwirq;
+ int err;
+ int i;
+
+ for (i = 0; i nr_irqs; i++) {
+ err = its_alloc_device_irq(its_dev, hwirq);
+ if (err)
+ return err;
+
+ err = its_irq_gic_domain_alloc(domain, virq + i, hwirq);
+ if (err)
+
Re: [patch 01/16] irqdomain: Introduce new interfaces to support hierarchy irqdomains
On 2014/11/24 22:32, Thomas Gleixner wrote: > On Mon, 24 Nov 2014, Yun Wu (Abel) wrote: >> On 2014/11/24 21:13, Thomas Gleixner wrote: >>> In the hierarchical case we do not touch the hardware in the >>> allocation step, so we need to activate the allocated interrupt in the >>> hardware before we can use it. And that's clearly a domain interface >>> not a irq chip issue. >>> >> >> Makes sense, now the interrupt domain seems to be the best place. >> And when the @domain parameter can be really useful? I haven't see >> anyone using it so far. > > All irqdomain callbacks take the domain pointer as their first > parameter. So for consistency sake we made it that way. > > You can argue in circles about whether the domain argument could be > removed. It's going to stay for now as it does not matter at all. > Yes, you are right. Thanks, Abel -- 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 01/16] irqdomain: Introduce new interfaces to support hierarchy irqdomains
On 2014/11/24 22:33, Jiang Liu wrote:
> On 2014/11/24 22:19, Yun Wu (Abel) wrote:
>> On 2014/11/24 22:11, Jiang Liu wrote:
>>
>>> On 2014/11/24 22:01, Yun Wu (Abel) wrote:
>>>> On 2014/11/24 21:13, Thomas Gleixner wrote:
>>>>
>>>>> On Mon, 24 Nov 2014, Yun Wu (Abel) wrote:
>>>>>> Hi Thomas, Jiang,
>>>>>> On 2014/11/12 21:42, Thomas Gleixner wrote:
>>>>>>
>>>>>>> From: Jiang Liu
>>>>>>>
>>>>>> [...]
>>>>>>> /* Number of irqs reserved for a legacy isa controller */
>>>>>>> #define NUM_ISA_INTERRUPTS 16
>>>>>>> @@ -64,6 +66,16 @@ struct irq_domain_ops {
>>>>>>> int (*xlate)(struct irq_domain *d, struct device_node *node,
>>>>>>> const u32 *intspec, unsigned int intsize,
>>>>>>> unsigned long *out_hwirq, unsigned int *out_type);
>>>>>>> +
>>>>>>> +#ifdef CONFIG_IRQ_DOMAIN_HIERARCHY
>>>>>>> + /* extended V2 interfaces to support hierarchy irq_domains */
>>>>>>> + int (*alloc)(struct irq_domain *d, unsigned int virq,
>>>>>>> +unsigned int nr_irqs, void *arg);
>>>>>>> + void (*free)(struct irq_domain *d, unsigned int virq,
>>>>>>> +unsigned int nr_irqs);
>>>>>>> + void (*activate)(struct irq_domain *d, struct irq_data
>>>>>>> *irq_data);
>>>>>>> + void (*deactivate)(struct irq_domain *d, struct irq_data
>>>>>>> *irq_data);
>>>>>>
>>>>>> What's the usage of the parameter domain reference in
>>>>>> activate/deactivate?
>>>>>> I think the purpose of the two callbacks is to activate/deactivate the
>>>>>> irq_data->hwirq in irq_data->domain. If so, the first parameter @domain
>>>>>> is
>>>>>> required to be equal to irq_data->domain (which makes @domain useless).
>>>>>> Besides, the main responsibility of interrupt domains is to manage
>>>>>> mappings
>>>>>> between hardware and linux interrupt numbers, so would it be better if
>>>>>> move
>>>>>> the two callbacks into struct irq_chip?
>>>>>
>>>>> No. It's not a function of the irq_chip to activate/deactivate a
>>>>> hierarchy. As I explained you before:
>>>>>
>>>>> The existing irqdomain code maps between hardware and virtual
>>>>> interrupts and thereby activates the interrupt in hardware.
>>>>>
>>>>> In the hierarchical case we do not touch the hardware in the
>>>>> allocation step, so we need to activate the allocated interrupt in the
>>>>> hardware before we can use it. And that's clearly a domain interface
>>>>> not a irq chip issue.
>>>>>
>>>>
>>>> Makes sense, now the interrupt domain seems to be the best place.
>>>> And when the @domain parameter can be really useful? I haven't see
>>>> anyone using it so far.
>>> We will use it for IOAPIC on x86, as below:
>>> void mp_irqdomain_deactivate(struct irq_domain *domain,
>>> struct irq_data *irq_data)
>>> {
>>> ioapic_mask_entry(mp_irqdomain_ioapic_idx(domain),
>>> (int)irq_data->hwirq);
>>> }
>>>
>>> >From an object oriented point of view, we pass the object as the
>>> first parameter. It's true that we could retrieve domain from
>>> irq_data->domain instead of explicitly passing it in, but that
>>> will cause irqdomain interfaces depends on irq_data, not sounds
>>> a good situation:)
>>
>> Hi Gerry,
>>
>> Is there any possibility that domain doesn't equal to irq_data->domain?
>> I'm a little confused..
> Hi Yun,
> Currently they are always the same, but we don't want irqdomain
> interfaces make assumption of struct irq_data. If it will bring big
> performance improvement, we will try to kill the first parameter,
> otherwise we may prefer keeping irqdomain interfaces clear.
OK, let's keep it as is. :)
Thanks,
Abel
--
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 01/16] irqdomain: Introduce new interfaces to support hierarchy irqdomains
On 2014/11/24 22:11, Jiang Liu wrote:
> On 2014/11/24 22:01, Yun Wu (Abel) wrote:
>> On 2014/11/24 21:13, Thomas Gleixner wrote:
>>
>>> On Mon, 24 Nov 2014, Yun Wu (Abel) wrote:
>>>> Hi Thomas, Jiang,
>>>> On 2014/11/12 21:42, Thomas Gleixner wrote:
>>>>
>>>>> From: Jiang Liu
>>>>>
>>>> [...]
>>>>> /* Number of irqs reserved for a legacy isa controller */
>>>>> #define NUM_ISA_INTERRUPTS 16
>>>>> @@ -64,6 +66,16 @@ struct irq_domain_ops {
>>>>> int (*xlate)(struct irq_domain *d, struct device_node *node,
>>>>>const u32 *intspec, unsigned int intsize,
>>>>>unsigned long *out_hwirq, unsigned int *out_type);
>>>>> +
>>>>> +#ifdef CONFIG_IRQ_DOMAIN_HIERARCHY
>>>>> + /* extended V2 interfaces to support hierarchy irq_domains */
>>>>> + int (*alloc)(struct irq_domain *d, unsigned int virq,
>>>>> + unsigned int nr_irqs, void *arg);
>>>>> + void (*free)(struct irq_domain *d, unsigned int virq,
>>>>> + unsigned int nr_irqs);
>>>>> + void (*activate)(struct irq_domain *d, struct irq_data *irq_data);
>>>>> + void (*deactivate)(struct irq_domain *d, struct irq_data *irq_data);
>>>>
>>>> What's the usage of the parameter domain reference in activate/deactivate?
>>>> I think the purpose of the two callbacks is to activate/deactivate the
>>>> irq_data->hwirq in irq_data->domain. If so, the first parameter @domain is
>>>> required to be equal to irq_data->domain (which makes @domain useless).
>>>> Besides, the main responsibility of interrupt domains is to manage mappings
>>>> between hardware and linux interrupt numbers, so would it be better if move
>>>> the two callbacks into struct irq_chip?
>>>
>>> No. It's not a function of the irq_chip to activate/deactivate a
>>> hierarchy. As I explained you before:
>>>
>>> The existing irqdomain code maps between hardware and virtual
>>> interrupts and thereby activates the interrupt in hardware.
>>>
>>> In the hierarchical case we do not touch the hardware in the
>>> allocation step, so we need to activate the allocated interrupt in the
>>> hardware before we can use it. And that's clearly a domain interface
>>> not a irq chip issue.
>>>
>>
>> Makes sense, now the interrupt domain seems to be the best place.
>> And when the @domain parameter can be really useful? I haven't see
>> anyone using it so far.
> We will use it for IOAPIC on x86, as below:
> void mp_irqdomain_deactivate(struct irq_domain *domain,
> struct irq_data *irq_data)
> {
> ioapic_mask_entry(mp_irqdomain_ioapic_idx(domain),
> (int)irq_data->hwirq);
> }
>
>>From an object oriented point of view, we pass the object as the
> first parameter. It's true that we could retrieve domain from
> irq_data->domain instead of explicitly passing it in, but that
> will cause irqdomain interfaces depends on irq_data, not sounds
> a good situation:)
Hi Gerry,
Is there any possibility that domain doesn't equal to irq_data->domain?
I'm a little confused..
Thanks,
Abel
--
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 01/16] irqdomain: Introduce new interfaces to support hierarchy irqdomains
On 2014/11/24 21:13, Thomas Gleixner wrote:
> On Mon, 24 Nov 2014, Yun Wu (Abel) wrote:
>> Hi Thomas, Jiang,
>> On 2014/11/12 21:42, Thomas Gleixner wrote:
>>
>>> From: Jiang Liu
>>>
>> [...]
>>> /* Number of irqs reserved for a legacy isa controller */
>>> #define NUM_ISA_INTERRUPTS 16
>>> @@ -64,6 +66,16 @@ struct irq_domain_ops {
>>> int (*xlate)(struct irq_domain *d, struct device_node *node,
>>> const u32 *intspec, unsigned int intsize,
>>> unsigned long *out_hwirq, unsigned int *out_type);
>>> +
>>> +#ifdef CONFIG_IRQ_DOMAIN_HIERARCHY
>>> + /* extended V2 interfaces to support hierarchy irq_domains */
>>> + int (*alloc)(struct irq_domain *d, unsigned int virq,
>>> +unsigned int nr_irqs, void *arg);
>>> + void (*free)(struct irq_domain *d, unsigned int virq,
>>> +unsigned int nr_irqs);
>>> + void (*activate)(struct irq_domain *d, struct irq_data *irq_data);
>>> + void (*deactivate)(struct irq_domain *d, struct irq_data *irq_data);
>>
>> What's the usage of the parameter domain reference in activate/deactivate?
>> I think the purpose of the two callbacks is to activate/deactivate the
>> irq_data->hwirq in irq_data->domain. If so, the first parameter @domain is
>> required to be equal to irq_data->domain (which makes @domain useless).
>> Besides, the main responsibility of interrupt domains is to manage mappings
>> between hardware and linux interrupt numbers, so would it be better if move
>> the two callbacks into struct irq_chip?
>
> No. It's not a function of the irq_chip to activate/deactivate a
> hierarchy. As I explained you before:
>
> The existing irqdomain code maps between hardware and virtual
> interrupts and thereby activates the interrupt in hardware.
>
> In the hierarchical case we do not touch the hardware in the
> allocation step, so we need to activate the allocated interrupt in the
> hardware before we can use it. And that's clearly a domain interface
> not a irq chip issue.
>
Makes sense, now the interrupt domain seems to be the best place.
And when the @domain parameter can be really useful? I haven't see
anyone using it so far.
Thanks,
Abel
--
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 01/16] irqdomain: Introduce new interfaces to support hierarchy irqdomains
Hi Thomas, Jiang,
On 2014/11/12 21:42, Thomas Gleixner wrote:
> From: Jiang Liu
>
[...]
> /* Number of irqs reserved for a legacy isa controller */
> #define NUM_ISA_INTERRUPTS 16
> @@ -64,6 +66,16 @@ struct irq_domain_ops {
> int (*xlate)(struct irq_domain *d, struct device_node *node,
>const u32 *intspec, unsigned int intsize,
>unsigned long *out_hwirq, unsigned int *out_type);
> +
> +#ifdef CONFIG_IRQ_DOMAIN_HIERARCHY
> + /* extended V2 interfaces to support hierarchy irq_domains */
> + int (*alloc)(struct irq_domain *d, unsigned int virq,
> + unsigned int nr_irqs, void *arg);
> + void (*free)(struct irq_domain *d, unsigned int virq,
> + unsigned int nr_irqs);
> + void (*activate)(struct irq_domain *d, struct irq_data *irq_data);
> + void (*deactivate)(struct irq_domain *d, struct irq_data *irq_data);
What's the usage of the parameter domain reference in activate/deactivate?
I think the purpose of the two callbacks is to activate/deactivate the
irq_data->hwirq in irq_data->domain. If so, the first parameter @domain is
required to be equal to irq_data->domain (which makes @domain useless).
Besides, the main responsibility of interrupt domains is to manage mappings
between hardware and linux interrupt numbers, so would it be better if move
the two callbacks into struct irq_chip?
Thanks,
Abel.
> +#endif
> };
>
> extern struct irq_domain_ops irq_generic_chip_ops;
> @@ -77,6 +89,7 @@ struct irq_domain_chip_generic;
> * @ops: pointer to irq_domain methods
> * @host_data: private data pointer for use by owner. Not touched by
> irq_domain
> * core code.
> + * @flags: host per irq_domain flags
> *
> * Optional elements
> * @of_node: Pointer to device tree nodes associated with the irq_domain.
> Used
> @@ -84,6 +97,7 @@ struct irq_domain_chip_generic;
> * @gc: Pointer to a list of generic chips. There is a helper function for
> * setting up one or more generic chips for interrupt controllers
> * drivers using the generic chip library which uses this pointer.
> + * @parent: Pointer to parent irq_domain to support hierarchy irq_domains
> *
> * Revmap data, used internally by irq_domain
> * @revmap_direct_max_irq: The largest hwirq that can be set for controllers
> that
> @@ -97,10 +111,14 @@ struct irq_domain {
> const char *name;
> const struct irq_domain_ops *ops;
> void *host_data;
> + unsigned int flags;
>
> /* Optional data */
> struct device_node *of_node;
> struct irq_domain_chip_generic *gc;
> +#ifdef CONFIG_IRQ_DOMAIN_HIERARCHY
> + struct irq_domain *parent;
> +#endif
>
> /* reverse map data. The linear map gets appended to the irq_domain */
> irq_hw_number_t hwirq_max;
> @@ -110,6 +128,9 @@ struct irq_domain {
> unsigned int linear_revmap[];
> };
>
> +#define IRQ_DOMAIN_FLAG_HIERARCHY 0x1
> +#define IRQ_DOMAIN_FLAG_ARCH1 0x1
> +
> #ifdef CONFIG_IRQ_DOMAIN
> struct irq_domain *__irq_domain_add(struct device_node *of_node, int size,
> irq_hw_number_t hwirq_max, int direct_max,
> @@ -220,8 +241,73 @@ int irq_domain_xlate_onetwocell(struct i
> const u32 *intspec, unsigned int intsize,
> irq_hw_number_t *out_hwirq, unsigned int *out_type);
>
> +/* V2 interfaces to support hierarchy IRQ domains. */
> +extern struct irq_data *irq_domain_get_irq_data(struct irq_domain *domain,
> + unsigned int virq);
> +#ifdef CONFIG_IRQ_DOMAIN_HIERARCHY
> +extern int __irq_domain_alloc_irqs(struct irq_domain *domain, int irq_base,
> +unsigned int nr_irqs, int node, void *arg,
> +bool realloc);
> +extern void irq_domain_free_irqs(unsigned int virq, unsigned int nr_irqs);
> +extern void irq_domain_activate_irq(struct irq_data *irq_data);
> +extern void irq_domain_deactivate_irq(struct irq_data *irq_data);
> +
> +static inline int irq_domain_alloc_irqs(struct irq_domain *domain,
> + unsigned int nr_irqs, int node, void *arg)
> +{
> + return __irq_domain_alloc_irqs(domain, -1, nr_irqs, node, arg, false);
> +}
> +
> +extern int irq_domain_set_hwirq_and_chip(struct irq_domain *domain,
> + unsigned int virq,
> + irq_hw_number_t hwirq,
> + struct irq_chip *chip,
> + void *chip_data);
> +extern void irq_domain_reset_irq_data(struct irq_data *irq_data);
> +extern void irq_domain_free_irqs_common(struct irq_domain *domain,
> + int virq, int nr_irqs);
> +extern void irq_domain_free_irqs_top(struct irq_domain *domain,
> + int virq,
Re: [patch 01/16] irqdomain: Introduce new interfaces to support hierarchy irqdomains
Hi Thomas, Jiang,
On 2014/11/12 21:42, Thomas Gleixner wrote:
From: Jiang Liu jiang@linux.intel.com
[...]
/* Number of irqs reserved for a legacy isa controller */
#define NUM_ISA_INTERRUPTS 16
@@ -64,6 +66,16 @@ struct irq_domain_ops {
int (*xlate)(struct irq_domain *d, struct device_node *node,
const u32 *intspec, unsigned int intsize,
unsigned long *out_hwirq, unsigned int *out_type);
+
+#ifdef CONFIG_IRQ_DOMAIN_HIERARCHY
+ /* extended V2 interfaces to support hierarchy irq_domains */
+ int (*alloc)(struct irq_domain *d, unsigned int virq,
+ unsigned int nr_irqs, void *arg);
+ void (*free)(struct irq_domain *d, unsigned int virq,
+ unsigned int nr_irqs);
+ void (*activate)(struct irq_domain *d, struct irq_data *irq_data);
+ void (*deactivate)(struct irq_domain *d, struct irq_data *irq_data);
What's the usage of the parameter domain reference in activate/deactivate?
I think the purpose of the two callbacks is to activate/deactivate the
irq_data-hwirq in irq_data-domain. If so, the first parameter @domain is
required to be equal to irq_data-domain (which makes @domain useless).
Besides, the main responsibility of interrupt domains is to manage mappings
between hardware and linux interrupt numbers, so would it be better if move
the two callbacks into struct irq_chip?
Thanks,
Abel.
+#endif
};
extern struct irq_domain_ops irq_generic_chip_ops;
@@ -77,6 +89,7 @@ struct irq_domain_chip_generic;
* @ops: pointer to irq_domain methods
* @host_data: private data pointer for use by owner. Not touched by
irq_domain
* core code.
+ * @flags: host per irq_domain flags
*
* Optional elements
* @of_node: Pointer to device tree nodes associated with the irq_domain.
Used
@@ -84,6 +97,7 @@ struct irq_domain_chip_generic;
* @gc: Pointer to a list of generic chips. There is a helper function for
* setting up one or more generic chips for interrupt controllers
* drivers using the generic chip library which uses this pointer.
+ * @parent: Pointer to parent irq_domain to support hierarchy irq_domains
*
* Revmap data, used internally by irq_domain
* @revmap_direct_max_irq: The largest hwirq that can be set for controllers
that
@@ -97,10 +111,14 @@ struct irq_domain {
const char *name;
const struct irq_domain_ops *ops;
void *host_data;
+ unsigned int flags;
/* Optional data */
struct device_node *of_node;
struct irq_domain_chip_generic *gc;
+#ifdef CONFIG_IRQ_DOMAIN_HIERARCHY
+ struct irq_domain *parent;
+#endif
/* reverse map data. The linear map gets appended to the irq_domain */
irq_hw_number_t hwirq_max;
@@ -110,6 +128,9 @@ struct irq_domain {
unsigned int linear_revmap[];
};
+#define IRQ_DOMAIN_FLAG_HIERARCHY 0x1
+#define IRQ_DOMAIN_FLAG_ARCH1 0x1
+
#ifdef CONFIG_IRQ_DOMAIN
struct irq_domain *__irq_domain_add(struct device_node *of_node, int size,
irq_hw_number_t hwirq_max, int direct_max,
@@ -220,8 +241,73 @@ int irq_domain_xlate_onetwocell(struct i
const u32 *intspec, unsigned int intsize,
irq_hw_number_t *out_hwirq, unsigned int *out_type);
+/* V2 interfaces to support hierarchy IRQ domains. */
+extern struct irq_data *irq_domain_get_irq_data(struct irq_domain *domain,
+ unsigned int virq);
+#ifdef CONFIG_IRQ_DOMAIN_HIERARCHY
+extern int __irq_domain_alloc_irqs(struct irq_domain *domain, int irq_base,
+unsigned int nr_irqs, int node, void *arg,
+bool realloc);
+extern void irq_domain_free_irqs(unsigned int virq, unsigned int nr_irqs);
+extern void irq_domain_activate_irq(struct irq_data *irq_data);
+extern void irq_domain_deactivate_irq(struct irq_data *irq_data);
+
+static inline int irq_domain_alloc_irqs(struct irq_domain *domain,
+ unsigned int nr_irqs, int node, void *arg)
+{
+ return __irq_domain_alloc_irqs(domain, -1, nr_irqs, node, arg, false);
+}
+
+extern int irq_domain_set_hwirq_and_chip(struct irq_domain *domain,
+ unsigned int virq,
+ irq_hw_number_t hwirq,
+ struct irq_chip *chip,
+ void *chip_data);
+extern void irq_domain_reset_irq_data(struct irq_data *irq_data);
+extern void irq_domain_free_irqs_common(struct irq_domain *domain,
+ int virq, int nr_irqs);
+extern void irq_domain_free_irqs_top(struct irq_domain *domain,
+ int virq, int nr_irqs);
+
+static inline int irq_domain_alloc_irqs_parent(struct
Re: [patch 01/16] irqdomain: Introduce new interfaces to support hierarchy irqdomains
On 2014/11/24 21:13, Thomas Gleixner wrote:
On Mon, 24 Nov 2014, Yun Wu (Abel) wrote:
Hi Thomas, Jiang,
On 2014/11/12 21:42, Thomas Gleixner wrote:
From: Jiang Liu jiang@linux.intel.com
[...]
/* Number of irqs reserved for a legacy isa controller */
#define NUM_ISA_INTERRUPTS 16
@@ -64,6 +66,16 @@ struct irq_domain_ops {
int (*xlate)(struct irq_domain *d, struct device_node *node,
const u32 *intspec, unsigned int intsize,
unsigned long *out_hwirq, unsigned int *out_type);
+
+#ifdef CONFIG_IRQ_DOMAIN_HIERARCHY
+ /* extended V2 interfaces to support hierarchy irq_domains */
+ int (*alloc)(struct irq_domain *d, unsigned int virq,
+unsigned int nr_irqs, void *arg);
+ void (*free)(struct irq_domain *d, unsigned int virq,
+unsigned int nr_irqs);
+ void (*activate)(struct irq_domain *d, struct irq_data *irq_data);
+ void (*deactivate)(struct irq_domain *d, struct irq_data *irq_data);
What's the usage of the parameter domain reference in activate/deactivate?
I think the purpose of the two callbacks is to activate/deactivate the
irq_data-hwirq in irq_data-domain. If so, the first parameter @domain is
required to be equal to irq_data-domain (which makes @domain useless).
Besides, the main responsibility of interrupt domains is to manage mappings
between hardware and linux interrupt numbers, so would it be better if move
the two callbacks into struct irq_chip?
No. It's not a function of the irq_chip to activate/deactivate a
hierarchy. As I explained you before:
The existing irqdomain code maps between hardware and virtual
interrupts and thereby activates the interrupt in hardware.
In the hierarchical case we do not touch the hardware in the
allocation step, so we need to activate the allocated interrupt in the
hardware before we can use it. And that's clearly a domain interface
not a irq chip issue.
Makes sense, now the interrupt domain seems to be the best place.
And when the @domain parameter can be really useful? I haven't see
anyone using it so far.
Thanks,
Abel
--
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 01/16] irqdomain: Introduce new interfaces to support hierarchy irqdomains
On 2014/11/24 22:11, Jiang Liu wrote:
On 2014/11/24 22:01, Yun Wu (Abel) wrote:
On 2014/11/24 21:13, Thomas Gleixner wrote:
On Mon, 24 Nov 2014, Yun Wu (Abel) wrote:
Hi Thomas, Jiang,
On 2014/11/12 21:42, Thomas Gleixner wrote:
From: Jiang Liu jiang@linux.intel.com
[...]
/* Number of irqs reserved for a legacy isa controller */
#define NUM_ISA_INTERRUPTS 16
@@ -64,6 +66,16 @@ struct irq_domain_ops {
int (*xlate)(struct irq_domain *d, struct device_node *node,
const u32 *intspec, unsigned int intsize,
unsigned long *out_hwirq, unsigned int *out_type);
+
+#ifdef CONFIG_IRQ_DOMAIN_HIERARCHY
+ /* extended V2 interfaces to support hierarchy irq_domains */
+ int (*alloc)(struct irq_domain *d, unsigned int virq,
+ unsigned int nr_irqs, void *arg);
+ void (*free)(struct irq_domain *d, unsigned int virq,
+ unsigned int nr_irqs);
+ void (*activate)(struct irq_domain *d, struct irq_data *irq_data);
+ void (*deactivate)(struct irq_domain *d, struct irq_data *irq_data);
What's the usage of the parameter domain reference in activate/deactivate?
I think the purpose of the two callbacks is to activate/deactivate the
irq_data-hwirq in irq_data-domain. If so, the first parameter @domain is
required to be equal to irq_data-domain (which makes @domain useless).
Besides, the main responsibility of interrupt domains is to manage mappings
between hardware and linux interrupt numbers, so would it be better if move
the two callbacks into struct irq_chip?
No. It's not a function of the irq_chip to activate/deactivate a
hierarchy. As I explained you before:
The existing irqdomain code maps between hardware and virtual
interrupts and thereby activates the interrupt in hardware.
In the hierarchical case we do not touch the hardware in the
allocation step, so we need to activate the allocated interrupt in the
hardware before we can use it. And that's clearly a domain interface
not a irq chip issue.
Makes sense, now the interrupt domain seems to be the best place.
And when the @domain parameter can be really useful? I haven't see
anyone using it so far.
We will use it for IOAPIC on x86, as below:
void mp_irqdomain_deactivate(struct irq_domain *domain,
struct irq_data *irq_data)
{
ioapic_mask_entry(mp_irqdomain_ioapic_idx(domain),
(int)irq_data-hwirq);
}
From an object oriented point of view, we pass the object as the
first parameter. It's true that we could retrieve domain from
irq_data-domain instead of explicitly passing it in, but that
will cause irqdomain interfaces depends on irq_data, not sounds
a good situation:)
Hi Gerry,
Is there any possibility that domain doesn't equal to irq_data-domain?
I'm a little confused..
Thanks,
Abel
--
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 01/16] irqdomain: Introduce new interfaces to support hierarchy irqdomains
On 2014/11/24 22:33, Jiang Liu wrote:
On 2014/11/24 22:19, Yun Wu (Abel) wrote:
On 2014/11/24 22:11, Jiang Liu wrote:
On 2014/11/24 22:01, Yun Wu (Abel) wrote:
On 2014/11/24 21:13, Thomas Gleixner wrote:
On Mon, 24 Nov 2014, Yun Wu (Abel) wrote:
Hi Thomas, Jiang,
On 2014/11/12 21:42, Thomas Gleixner wrote:
From: Jiang Liu jiang@linux.intel.com
[...]
/* Number of irqs reserved for a legacy isa controller */
#define NUM_ISA_INTERRUPTS 16
@@ -64,6 +66,16 @@ struct irq_domain_ops {
int (*xlate)(struct irq_domain *d, struct device_node *node,
const u32 *intspec, unsigned int intsize,
unsigned long *out_hwirq, unsigned int *out_type);
+
+#ifdef CONFIG_IRQ_DOMAIN_HIERARCHY
+ /* extended V2 interfaces to support hierarchy irq_domains */
+ int (*alloc)(struct irq_domain *d, unsigned int virq,
+unsigned int nr_irqs, void *arg);
+ void (*free)(struct irq_domain *d, unsigned int virq,
+unsigned int nr_irqs);
+ void (*activate)(struct irq_domain *d, struct irq_data
*irq_data);
+ void (*deactivate)(struct irq_domain *d, struct irq_data
*irq_data);
What's the usage of the parameter domain reference in
activate/deactivate?
I think the purpose of the two callbacks is to activate/deactivate the
irq_data-hwirq in irq_data-domain. If so, the first parameter @domain
is
required to be equal to irq_data-domain (which makes @domain useless).
Besides, the main responsibility of interrupt domains is to manage
mappings
between hardware and linux interrupt numbers, so would it be better if
move
the two callbacks into struct irq_chip?
No. It's not a function of the irq_chip to activate/deactivate a
hierarchy. As I explained you before:
The existing irqdomain code maps between hardware and virtual
interrupts and thereby activates the interrupt in hardware.
In the hierarchical case we do not touch the hardware in the
allocation step, so we need to activate the allocated interrupt in the
hardware before we can use it. And that's clearly a domain interface
not a irq chip issue.
Makes sense, now the interrupt domain seems to be the best place.
And when the @domain parameter can be really useful? I haven't see
anyone using it so far.
We will use it for IOAPIC on x86, as below:
void mp_irqdomain_deactivate(struct irq_domain *domain,
struct irq_data *irq_data)
{
ioapic_mask_entry(mp_irqdomain_ioapic_idx(domain),
(int)irq_data-hwirq);
}
From an object oriented point of view, we pass the object as the
first parameter. It's true that we could retrieve domain from
irq_data-domain instead of explicitly passing it in, but that
will cause irqdomain interfaces depends on irq_data, not sounds
a good situation:)
Hi Gerry,
Is there any possibility that domain doesn't equal to irq_data-domain?
I'm a little confused..
Hi Yun,
Currently they are always the same, but we don't want irqdomain
interfaces make assumption of struct irq_data. If it will bring big
performance improvement, we will try to kill the first parameter,
otherwise we may prefer keeping irqdomain interfaces clear.
OK, let's keep it as is. :)
Thanks,
Abel
--
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 01/16] irqdomain: Introduce new interfaces to support hierarchy irqdomains
On 2014/11/24 22:32, Thomas Gleixner wrote: On Mon, 24 Nov 2014, Yun Wu (Abel) wrote: On 2014/11/24 21:13, Thomas Gleixner wrote: In the hierarchical case we do not touch the hardware in the allocation step, so we need to activate the allocated interrupt in the hardware before we can use it. And that's clearly a domain interface not a irq chip issue. Makes sense, now the interrupt domain seems to be the best place. And when the @domain parameter can be really useful? I haven't see anyone using it so far. All irqdomain callbacks take the domain pointer as their first parameter. So for consistency sake we made it that way. You can argue in circles about whether the domain argument could be removed. It's going to stay for now as it does not matter at all. Yes, you are right. Thanks, Abel -- 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 08/16] genirq: Introduce callback irq_chip.irq_write_msi_msg
On 2014/11/18 22:32, Thomas Gleixner wrote: > On Tue, 18 Nov 2014, Yun Wu (Abel) wrote: > > Can you please trim the messages when you're replying? > >> The above you described is absolutely right, but not the things I want >> to know. :) >> Take GICv3 ITS for example, it deals with both PCI and non PCI message >> interrupts. IIUC, several irq_chips need to be implemented in the ITS >> driver (i.e. pci_msi_chip, A_msi_chip and B_msi_chip). What should we >> do to the ITS driver if new MSI-capable devices come out? > > You seem to miss the stacking here > > PCI-MSI -> > A-MSI -> ITS -> GIC > B-MSI -> > > So each of the device types has its own MSI controller. Each of them > will have their own callbacks and are backed by the underlying ITS/GIC > implementation. Yes, this hits the key point. Once a new device type becomes available, we need to add pieces of code outside the new device's driver to make it work, which in my opinion is due to lack of core infrastructure. More specifically, the core infrastructure needs to support mechanism of MSI, not the various types of devices. > > And that's the only sensible solution. > It's sensible, but not perfect. What I suggested is to add a generic layer: PCI-MSI -> A-MSI -> (generic layer) -> ITS -> GICR B-MSI -> The PCI/A/B/... passes its hardware properties to the generic layer which gets configurations ready by calling ITS's domain/chip callbacks. When a new device type arrives, the only thing need to do is to implement the driver of that device, with nothing to do with the generic layer or ITS. Thanks, Abel -- 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 08/16] genirq: Introduce callback irq_chip.irq_write_msi_msg
On 2014/11/18 22:52, Jiang Liu wrote: > On 2014/11/18 22:34, Yun Wu (Abel) wrote: >> On 2014/11/18 22:19, Thomas Gleixner wrote: >> >>> On Tue, 18 Nov 2014, Yun Wu (Abel) wrote: >>>> On 2014/11/18 21:43, Jiang Liu wrote: >>>>> We provide an irq_chip for each type of interrupt controller >>>>> instead of devices. For the example mentioned above, if device A >>>>> and Group B has different interrupt controllers, we just need to >>>>> implement irq_chip_A and irq_chip_B and set irq_chip.irq_write_msi_msg() >>>>> to suitable callbacks. >>>>> The framework already achieves what you you want:) >>>> >>>> What if device A and group B have the same interrupt controller? >>> >>> Well, if write_msg() is different they are hardly the same. >>> >> >> The GICv3 ITS now deals with both PCI and non PCI message interrupts. >> We can't require the new devices behave writing message in a same way. >> What we can do is to abstract all the endpoints' behavior, and I >> provided one abstraction in an earlier reply. > It should be easy to extend:) > Actually, x86 interrupt remapping drivers already support two types of > MSIs, one is PCI MSI/MSIX, another is HPET interrupt. Well, if there are one hundred types, I don't think it's as easy as you thought to extend. Of course we can doubt the possibility of being hundred, but tens or twenties is reasonably possible lying under the fact we have already startet to integrate the MSI registers (or some other form to store information) into the individual devices. Thanks, Abel -- 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 08/16] genirq: Introduce callback irq_chip.irq_write_msi_msg
On 2014/11/19 1:21, Marc Zyngier wrote: > On Tue, Nov 18 2014 at 2:34:44 pm GMT, "Yun Wu (Abel)" > wrote: >> On 2014/11/18 22:19, Thomas Gleixner wrote: >> >>> On Tue, 18 Nov 2014, Yun Wu (Abel) wrote: >>>> On 2014/11/18 21:43, Jiang Liu wrote: >>>>> We provide an irq_chip for each type of interrupt controller >>>>> instead of devices. For the example mentioned above, if device A >>>>> and Group B has different interrupt controllers, we just need to >>>>> implement irq_chip_A and irq_chip_B and set irq_chip.irq_write_msi_msg() >>>>> to suitable callbacks. >>>>> The framework already achieves what you you want:) >>>> >>>> What if device A and group B have the same interrupt controller? >>> >>> Well, if write_msg() is different they are hardly the same. >>> >> >> The GICv3 ITS now deals with both PCI and non PCI message interrupts. >> We can't require the new devices behave writing message in a same way. >> What we can do is to abstract all the endpoints' behavior, and I >> provided one abstraction in an earlier reply. > > This is why the framework gives you the opportunity to provide methods > that: > - compose the message > - program the message into the device > > None of that has to be PCI specific, and gives you a clean > abstraction. The framework only gives you a number of shortcuts for PCI > MSI, because that's what most people care about. > Indeed, and I never said Jiang's patches don't work, I was just thinking that they were not that perfect. Thanks, Abel -- 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 08/16] genirq: Introduce callback irq_chip.irq_write_msi_msg
On 2014/11/19 1:14, Marc Zyngier wrote: > On Tue, Nov 18 2014 at 2:46:02 pm GMT, "Yun Wu (Abel)" > wrote: [...] >> IIUC, Marc's patch now only supports PCI MSI/MSI-X... > > Indeed, and the current solution makes is relatively easy to plug in > non-PCI MSI. Just don't plug the ITS into the *PCI* MSI framework when > you encounter such a thing. > I am looking forward to that. :) I guess the main reason you haven't plugged in non-PCI MSI is the way of obtaining device ids used in searching DT table. Thanks, Abel -- 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 08/16] genirq: Introduce callback irq_chip.irq_write_msi_msg
On 2014/11/18 22:29, Jiang Liu wrote:
>
>
> On 2014/11/18 22:22, Yun Wu (Abel) wrote:
>> On 2014/11/18 22:03, Jiang Liu wrote:
>>
>>> On 2014/11/18 21:52, Yun Wu (Abel) wrote:
>>>> On 2014/11/18 21:43, Jiang Liu wrote:
>>>>
>>>>> On 2014/11/18 21:33, Yun Wu (Abel) wrote:
>>>>>> On 2014/11/18 18:19, Thomas Gleixner wrote:
>>>>>>
>>>>>>> On Tue, 18 Nov 2014, Yun Wu (Abel) wrote:
>>>>>>>> On 2014/11/12 21:43, Thomas Gleixner wrote:
>>>>>>>>> struct irq_chip {
>>>>>>>>> @@ -359,6 +360,7 @@ struct irq_chip {
>>>>>>>>> void(*irq_release_resources)(struct irq_data *data);
>>>>>>>>>
>>>>>>>>> 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);
>>>>>>>>
>>>>>>>> Hmm... It's really weird.
>>>>>>>> I don't think it's the interrupt controllers' responsibility to write
>>>>>>>> messages
>>>>>>>> for all the endpoint devices since the methods of configuring message
>>>>>>>> registers
>>>>>>>> may different between these devices. And theoretically, the endpoint
>>>>>>>> devices
>>>>>>>> themselves should take the responsibility to configure their message
>>>>>>>> registers.
>>>>>>>> To say the least, the write_msg callback here still need to call some
>>>>>>>> certain
>>>>>>>> interfaces provided by the corresponding device.
>>>>>>>>
>>>>>>>> There would be lots of ARM new devices capable of sending message
>>>>>>>> based interrupts to interrupt controllers, does all the drivers of
>>>>>>>> the devices need to expose a write_msg callback to interrupt
>>>>>>>> controllers?
>>>>>>>
>>>>>>> Well, writing the message _IS_ part of the interrupt controller.
>>>>>>>
>>>>>>> So in order to enable non PCI based MSI we want to have generic
>>>>>>> infrastructure with minimal per device/device class callbacks and of
>>>>>>> course you need to provide that callback for your special device.
>>>>>>>
>>>>>>> We already have non PCI based MSI controllers in x86 today and we need
>>>>>>> to handle the whole stuff with tons of copied coded extra for each of
>>>>>>> those. So consolidating it into common infrastructure allows us to get
>>>>>>> rid of the pointless copied code and reduce the per device effort to
>>>>>>> the relevant hardware specific callbacks. irq_write_msi_msg being one
>>>>>>> of those.
>>>>>>>
>>>>>>
>>>>>> At least, we have the same goal.
>>>>>> I will illustrate my thoughts by an example.
>>>>>> The current code is something like:
>>>>>>
>>>>>> Device A
>>>>>>
>>>>>> void A_write_msg() { ... }
>>>>>>
>>>>>> Group B
>>>>>> (a group of devices behave same on writing messages, i.e. PCI)
>>>>>> ===
>>>>>> void B_write_msg() { ... }
>>>>>>
>>>>>> Controller
>>>>>> ==
>>>>>> irq_chip.irq_write_msi_msg () {
>>>>>> if (A)
>>>>>> A_write_msg();
>>>>>> if (B)
>>>>>> B_write_msg();
>>>>>> }
>>>>>>
>>>>>> It's horrible when new devices come out, since we need to modify the
>>>>>> controller part for each new device.
>>>>>> What I suggested is:
>>>>>>
>>>>>> MSI Core
>>>>>>
>>>>>> struct msi_ops { .write_msg, };
>>>>>> struct msi_desc { .msi_ops, };
>>>>>>
>>>>>> write_msg() {
>>>>>> X = get_dev();
>>>>&
Re: [patch 08/16] genirq: Introduce callback irq_chip.irq_write_msi_msg
On 2014/11/18 22:19, Thomas Gleixner wrote: > On Tue, 18 Nov 2014, Yun Wu (Abel) wrote: >> On 2014/11/18 21:43, Jiang Liu wrote: >>> We provide an irq_chip for each type of interrupt controller >>> instead of devices. For the example mentioned above, if device A >>> and Group B has different interrupt controllers, we just need to >>> implement irq_chip_A and irq_chip_B and set irq_chip.irq_write_msi_msg() >>> to suitable callbacks. >>> The framework already achieves what you you want:) >> >> What if device A and group B have the same interrupt controller? > > Well, if write_msg() is different they are hardly the same. > The GICv3 ITS now deals with both PCI and non PCI message interrupts. We can't require the new devices behave writing message in a same way. What we can do is to abstract all the endpoints' behavior, and I provided one abstraction in an earlier reply. Thanks, Abel -- 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 09/16] genirq: Add generic msi irq domain support
On 2014/11/18 22:24, Thomas Gleixner wrote:
> On Tue, 18 Nov 2014, Yun Wu (Abel) wrote:
>> On 2014/11/18 20:49, Jiang Liu wrote:
>>>>> + ret = irq_domain_alloc_irqs_parent(domain, virq, nr_irqs, arg);
>>>>> + if (ret < 0)
>>>>> + return ret;
>>>>> +
>>>>> + for (i = 0; i < nr_irqs; i++) {
>>>>> + ret = ops->msi_init(domain, info, virq + i, hwirq + i, arg);
>>>>> + if (ret < 0) {
>>>>> + if (ops->msi_free) {
>>>>> + for (i--; i > 0; i--)
>>>>
>>>> while (i--) ?
>
> The allocation for 'i' failed. So we don't free i. You missed the real
> bug here:
>
>>>>> + for (i--; i > 0; i--)
>
> Must be i >= 0.
>
"for (i--; i >= 0; i--)" is just the same as "while (i--)".
Thanks,
Abel
--
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 08/16] genirq: Introduce callback irq_chip.irq_write_msi_msg
On 2014/11/18 22:03, Jiang Liu wrote:
> On 2014/11/18 21:52, Yun Wu (Abel) wrote:
>> On 2014/11/18 21:43, Jiang Liu wrote:
>>
>>> On 2014/11/18 21:33, Yun Wu (Abel) wrote:
>>>> On 2014/11/18 18:19, Thomas Gleixner wrote:
>>>>
>>>>> On Tue, 18 Nov 2014, Yun Wu (Abel) wrote:
>>>>>> On 2014/11/12 21:43, Thomas Gleixner wrote:
>>>>>>> struct irq_chip {
>>>>>>> @@ -359,6 +360,7 @@ struct irq_chip {
>>>>>>> void(*irq_release_resources)(struct irq_data *data);
>>>>>>>
>>>>>>> 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);
>>>>>>
>>>>>> Hmm... It's really weird.
>>>>>> I don't think it's the interrupt controllers' responsibility to write
>>>>>> messages
>>>>>> for all the endpoint devices since the methods of configuring message
>>>>>> registers
>>>>>> may different between these devices. And theoretically, the endpoint
>>>>>> devices
>>>>>> themselves should take the responsibility to configure their message
>>>>>> registers.
>>>>>> To say the least, the write_msg callback here still need to call some
>>>>>> certain
>>>>>> interfaces provided by the corresponding device.
>>>>>>
>>>>>> There would be lots of ARM new devices capable of sending message
>>>>>> based interrupts to interrupt controllers, does all the drivers of
>>>>>> the devices need to expose a write_msg callback to interrupt
>>>>>> controllers?
>>>>>
>>>>> Well, writing the message _IS_ part of the interrupt controller.
>>>>>
>>>>> So in order to enable non PCI based MSI we want to have generic
>>>>> infrastructure with minimal per device/device class callbacks and of
>>>>> course you need to provide that callback for your special device.
>>>>>
>>>>> We already have non PCI based MSI controllers in x86 today and we need
>>>>> to handle the whole stuff with tons of copied coded extra for each of
>>>>> those. So consolidating it into common infrastructure allows us to get
>>>>> rid of the pointless copied code and reduce the per device effort to
>>>>> the relevant hardware specific callbacks. irq_write_msi_msg being one
>>>>> of those.
>>>>>
>>>>
>>>> At least, we have the same goal.
>>>> I will illustrate my thoughts by an example.
>>>> The current code is something like:
>>>>
>>>> Device A
>>>>
>>>> void A_write_msg() { ... }
>>>>
>>>> Group B
>>>> (a group of devices behave same on writing messages, i.e. PCI)
>>>> ===
>>>> void B_write_msg() { ... }
>>>>
>>>> Controller
>>>> ==
>>>> irq_chip.irq_write_msi_msg () {
>>>>if (A)
>>>>A_write_msg();
>>>>if (B)
>>>>B_write_msg();
>>>> }
>>>>
>>>> It's horrible when new devices come out, since we need to modify the
>>>> controller part for each new device.
>>>> What I suggested is:
>>>>
>>>> MSI Core
>>>>
>>>> struct msi_ops { .write_msg, };
>>>> struct msi_desc { .msi_ops, };
>>>>
>>>> write_msg() {
>>>>X = get_dev();
>>>>irq_chip.compose_msg(X);// IRQ chips' responsibility
>>>>X_msi_ops.write_msg(); // nothing to do with IRQ chips
>>>> }
>>>>
>>>> Device A
>>>>
>>>> void A_write_msg() { ... }
>>>> A_msi_ops.write_msg = A_write_msg;
>>>>
>>>> Group B
>>>> ===
>>>> void B_write_msg() { ... }
>>>> B_msi_ops.write_msg = B_write_msg;
>>>>
>>>> Please correct me if I misunderstood anything.
>>> Hi Yun,
>>> We provide an irq_chip for each type of interrupt controller
>>> instead of devices. For the example mentioned above, if device A
>>>
Re: [patch 04/16] genirq: Introduce irq_chip.irq_compose_msi_msg() to support stacked irqchip
On 2014/11/18 21:55, Jiang Liu wrote:
> On 2014/11/18 21:48, Yun Wu (Abel) wrote:
>> On 2014/11/18 21:25, Jiang Liu wrote:
>>
>>> On 2014/11/18 21:16, Yun Wu (Abel) wrote:
>>>> On 2014/11/18 20:43, Jiang Liu wrote:
>>>>
>>>>> On 2014/11/18 19:47, Yun Wu (Abel) wrote:
>>>>>> On 2014/11/18 18:02, Thomas Gleixner wrote:
>>>>>>
>>>>>>> On Tue, 18 Nov 2014, Yun Wu (Abel) wrote:
>>>>>>>> On 2014/11/12 21:42, Thomas Gleixner wrote:
>>>>>>>>> +int irq_chip_compose_msi_msg(struct irq_data *data, struct msi_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_msi_msg)
>>>>>>>>> + pos = data;
>>>>>>>>> + if (!pos)
>>>>>>>>> + return -ENOSYS;
>>>>>>>>> +
>>>>>>>>> + pos->chip->irq_compose_msi_msg(pos, msg);
>>>>>>>>> +
>>>>>>>>> + return 0;
>>>>>>>>> +}
>>>>>>>>
>>>>>>>> Adding message composing routine to struct irq_chip is OK to me, and
>>>>>>>> it should
>>>>>>>> be because it is interrupt controllers' duty to compose messages (so
>>>>>>>> that they
>>>>>>>> can parse the messages correctly without any pre-defined rules that
>>>>>>>> endpoint
>>>>>>>> devices absolutely need not to know).
>>>>>>>> However a problem comes out when deciding which parameters should be
>>>>>>>> passed to
>>>>>>>> this routine. A message can associate with multiple interrupts, which
>>>>>>>> makes me
>>>>>>>> think composing messages for each interrupt is not that appropriate.
>>>>>>>> And we
>>>>>>>> can take a look at the new routine irq_chip_compose_msi_msg(). It is
>>>>>>>> called by
>>>>>>>> msi_domain_activate() which will be called by
>>>>>>>> irq_domain_activate_irq() in
>>>>>>>> irq_startup() for each interrupt descriptor, result in composing a
>>>>>>>> message for
>>>>>>>> each interrupt, right? (Unless requiring a judge on the parameter
>>>>>>>> @data when
>>>>>>>> implementing the irq_compose_msi_msg() callback that only compose
>>>>>>>> message for
>>>>>>>> the first entry of that message. But I really don't like that...)
>>>>>>>
>>>>>>> No, that's not correct. You are looking at some random stale version
>>>>>>> of this. The current state of affairs is in
>>>>>>>
>>>>>>>git://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git
>>>>>>> irq/irqdomain
>>>>>>>
>>>>>>> See also https://lkml.org/lkml/2014/11/17/764
>>>>>>>
>>>>>>> In activate we write the message, which is the right point to do so.
>>>>>>>
>>>>>>
>>>>>> I checked the current state, it seems to be the same.
>>>>>> Yes, the decision of postponing the actual hardware programming to the
>>>>>> point
>>>>>> where the interrupt actually gets used is right, but here above I was
>>>>>> talking
>>>>>> another thing.
>>>>>> As I mentioned, a message can associate with multiple interrupts.
>>>>>> Enabling
>>>>>> any of them will call irq_startup(). So if we don't want to compose or
>>>>>> write
>>>>>> messages repeatedly, we'd better require performing some checks before
>>>>>> activating the interrupts.
>>>>> Hi Yun,
>>>>> Seems you are talking about the case of multiple MSI support.
>
Re: [patch 09/16] genirq: Add generic msi irq domain support
On 2014/11/18 20:49, Jiang Liu wrote:
>
> On 2014/11/18 20:07, Yun Wu (Abel) wrote:
>> On 2014/11/12 21:43, Thomas Gleixner wrote:
>>
>>> From: Jiang Liu
>
>>> +static int msi_domain_alloc(struct irq_domain *domain, unsigned int virq,
>>> + unsigned int nr_irqs, void *arg)
>>> +{
>>> + struct msi_domain_info *info = domain->host_data;
>>> + struct msi_domain_ops *ops = info->ops;
>>> + irq_hw_number_t hwirq = ops->get_hwirq(info, arg);
>>> + int i, ret;
>>> +
>>> + if (irq_find_mapping(domain, hwirq) > 0)
>>> + return -EEXIST;
>>
>> I think we need to check range here.
>> I fixed this by applying a patch showed in the bottom.
> Hi Yun,
> Yes, we have some assumptions here about the way the interface
> will be used for. Will improve it in future patches.
> Regards!
> Gerry
>>
OK. Just note that don't forget to check my second inline comment below. :)
>>> +
>>> + ret = irq_domain_alloc_irqs_parent(domain, virq, nr_irqs, arg);
>>> + if (ret < 0)
>>> + return ret;
>>> +
>>> + for (i = 0; i < nr_irqs; i++) {
>>> + ret = ops->msi_init(domain, info, virq + i, hwirq + i, arg);
>>> + if (ret < 0) {
>>> + if (ops->msi_free) {
>>> + for (i--; i > 0; i--)
>>
>> while (i--) ?
>>
Here.
Thanks,
Abel
>>
>>> + ops->msi_free(domain, info, virq + i);
>>> + }
>>> + irq_domain_free_irqs_top(domain, virq, nr_irqs);
>>> + return ret;
>>> + }
>>> + }
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +static void msi_domain_free(struct irq_domain *domain, unsigned int virq,
>>> + unsigned int nr_irqs)
>>> +{
>>> + struct msi_domain_info *info = domain->host_data;
>>> + int i;
>>> +
>>> + if (info->ops->msi_free) {
>>> + for (i = 0; i < nr_irqs; i++)
>>> + info->ops->msi_free(domain, info, virq + i);
>>> + }
>>> + irq_domain_free_irqs_top(domain, virq, nr_irqs);
>>> +}
>>> +
>>> +static struct irq_domain_ops msi_domain_ops = {
>>> + .alloc = msi_domain_alloc,
>>> + .free = msi_domain_free,
>>> + .activate = msi_domain_activate,
>>> + .deactivate = msi_domain_deactivate,
>>> +};
>>> +
>>> +struct irq_domain *msi_create_irq_domain(struct device_node *of_node,
>>> +struct msi_domain_info *info,
>>> +struct irq_domain *parent)
>>> +{
>>> + struct irq_domain *domain;
>>> +
>>> + domain = irq_domain_add_tree(of_node, _domain_ops, info);
>>> + if (domain)
>>> + domain->parent = parent;
>>> +
>>> + return domain;
>>> +}
>>> +
>>> +struct msi_domain_info *msi_get_domain_info(struct irq_domain *domain)
>>> +{
>>> + return (struct msi_domain_info *)domain->host_data;
>>> +}
>>> +
>>> +#endif /* CONFIG_GENERIC_MSI_IRQ_DOMAIN */
>>>
>>>
>>
>> diff --git a/kernel/irq/irqdomain.c b/kernel/irq/irqdomain.c
>> index cc18182..6d1be00 100644
>> --- a/kernel/irq/irqdomain.c
>> +++ b/kernel/irq/irqdomain.c
>> @@ -568,6 +568,34 @@ unsigned int irq_find_mapping(struct irq_domain *domain,
>> }
>> EXPORT_SYMBOL_GPL(irq_find_mapping);
>>
>> +/**
>> + * irq_find_mapping_many() - Find a range of linux irqs from a range of
>> hwirqs.
>> + * @domain: domain owning these hardware interrupts
>> + * @hwirq: start hardware irq number in @domain space
>> + * @count: number of interrupts to find
>> + *
>> + * Returns the first linux irq number on success, or 0 when not found, or
>> error.
>> + */
>> +int irq_find_mapping_many(struct irq_domain *domain, irq_hw_number_t hwirq,
>> + int count)
>> +{
>> +unsigned int from;
>> +int i;
>> +
>> +for (i = from = 0; i < count; i++) {
>> +if (!from) {
>> +from = irq_find_mapping(domain, hwirq + i);
>> +if (from && i)
>> +return -1;
>> +} else if ((from + i) != irq_find_mapping(domain, hwirq + i)) {
>> +return -1;
>> +}
>> +}
>> +
>> +return from;
>> +}
>> +EXPORT_SYMBOL_GPL(irq_find_mapping_many);
>> +
>> #ifdef CONFIG_IRQ_DOMAIN_DEBUG
>> static int virq_debug_show(struct seq_file *m, void *private)
>> {
>>
>>
>>
>
> .
>
--
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 08/16] genirq: Introduce callback irq_chip.irq_write_msi_msg
On 2014/11/18 21:43, Jiang Liu wrote:
> On 2014/11/18 21:33, Yun Wu (Abel) wrote:
>> On 2014/11/18 18:19, Thomas Gleixner wrote:
>>
>>> On Tue, 18 Nov 2014, Yun Wu (Abel) wrote:
>>>> On 2014/11/12 21:43, Thomas Gleixner wrote:
>>>>> struct irq_chip {
>>>>> @@ -359,6 +360,7 @@ struct irq_chip {
>>>>> void(*irq_release_resources)(struct irq_data *data);
>>>>>
>>>>> 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);
>>>>
>>>> Hmm... It's really weird.
>>>> I don't think it's the interrupt controllers' responsibility to write
>>>> messages
>>>> for all the endpoint devices since the methods of configuring message
>>>> registers
>>>> may different between these devices. And theoretically, the endpoint
>>>> devices
>>>> themselves should take the responsibility to configure their message
>>>> registers.
>>>> To say the least, the write_msg callback here still need to call some
>>>> certain
>>>> interfaces provided by the corresponding device.
>>>>
>>>> There would be lots of ARM new devices capable of sending message
>>>> based interrupts to interrupt controllers, does all the drivers of
>>>> the devices need to expose a write_msg callback to interrupt
>>>> controllers?
>>>
>>> Well, writing the message _IS_ part of the interrupt controller.
>>>
>>> So in order to enable non PCI based MSI we want to have generic
>>> infrastructure with minimal per device/device class callbacks and of
>>> course you need to provide that callback for your special device.
>>>
>>> We already have non PCI based MSI controllers in x86 today and we need
>>> to handle the whole stuff with tons of copied coded extra for each of
>>> those. So consolidating it into common infrastructure allows us to get
>>> rid of the pointless copied code and reduce the per device effort to
>>> the relevant hardware specific callbacks. irq_write_msi_msg being one
>>> of those.
>>>
>>
>> At least, we have the same goal.
>> I will illustrate my thoughts by an example.
>> The current code is something like:
>>
>> Device A
>>
>> void A_write_msg() { ... }
>>
>> Group B
>> (a group of devices behave same on writing messages, i.e. PCI)
>> ===
>> void B_write_msg() { ... }
>>
>> Controller
>> ==
>> irq_chip.irq_write_msi_msg () {
>> if (A)
>> A_write_msg();
>> if (B)
>> B_write_msg();
>> }
>>
>> It's horrible when new devices come out, since we need to modify the
>> controller part for each new device.
>> What I suggested is:
>>
>> MSI Core
>>
>> struct msi_ops { .write_msg, };
>> struct msi_desc { .msi_ops, };
>>
>> write_msg() {
>> X = get_dev();
>> irq_chip.compose_msg(X);// IRQ chips' responsibility
>> X_msi_ops.write_msg(); // nothing to do with IRQ chips
>> }
>>
>> Device A
>>
>> void A_write_msg() { ... }
>> A_msi_ops.write_msg = A_write_msg;
>>
>> Group B
>> ===
>> void B_write_msg() { ... }
>> B_msi_ops.write_msg = B_write_msg;
>>
>> Please correct me if I misunderstood anything.
> Hi Yun,
> We provide an irq_chip for each type of interrupt controller
> instead of devices. For the example mentioned above, if device A
> and Group B has different interrupt controllers, we just need to
> implement irq_chip_A and irq_chip_B and set irq_chip.irq_write_msi_msg()
> to suitable callbacks.
> The framework already achieves what you you want:)
What if device A and group B have the same interrupt controller?
Regards,
Abel
--
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 04/16] genirq: Introduce irq_chip.irq_compose_msi_msg() to support stacked irqchip
On 2014/11/18 21:25, Jiang Liu wrote:
> On 2014/11/18 21:16, Yun Wu (Abel) wrote:
>> On 2014/11/18 20:43, Jiang Liu wrote:
>>
>>> On 2014/11/18 19:47, Yun Wu (Abel) wrote:
>>>> On 2014/11/18 18:02, Thomas Gleixner wrote:
>>>>
>>>>> On Tue, 18 Nov 2014, Yun Wu (Abel) wrote:
>>>>>> On 2014/11/12 21:42, Thomas Gleixner wrote:
>>>>>>> +int irq_chip_compose_msi_msg(struct irq_data *data, struct msi_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_msi_msg)
>>>>>>> + pos = data;
>>>>>>> + if (!pos)
>>>>>>> + return -ENOSYS;
>>>>>>> +
>>>>>>> + pos->chip->irq_compose_msi_msg(pos, msg);
>>>>>>> +
>>>>>>> + return 0;
>>>>>>> +}
>>>>>>
>>>>>> Adding message composing routine to struct irq_chip is OK to me, and it
>>>>>> should
>>>>>> be because it is interrupt controllers' duty to compose messages (so
>>>>>> that they
>>>>>> can parse the messages correctly without any pre-defined rules that
>>>>>> endpoint
>>>>>> devices absolutely need not to know).
>>>>>> However a problem comes out when deciding which parameters should be
>>>>>> passed to
>>>>>> this routine. A message can associate with multiple interrupts, which
>>>>>> makes me
>>>>>> think composing messages for each interrupt is not that appropriate. And
>>>>>> we
>>>>>> can take a look at the new routine irq_chip_compose_msi_msg(). It is
>>>>>> called by
>>>>>> msi_domain_activate() which will be called by irq_domain_activate_irq()
>>>>>> in
>>>>>> irq_startup() for each interrupt descriptor, result in composing a
>>>>>> message for
>>>>>> each interrupt, right? (Unless requiring a judge on the parameter @data
>>>>>> when
>>>>>> implementing the irq_compose_msi_msg() callback that only compose
>>>>>> message for
>>>>>> the first entry of that message. But I really don't like that...)
>>>>>
>>>>> No, that's not correct. You are looking at some random stale version
>>>>> of this. The current state of affairs is in
>>>>>
>>>>>git://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git irq/irqdomain
>>>>>
>>>>> See also https://lkml.org/lkml/2014/11/17/764
>>>>>
>>>>> In activate we write the message, which is the right point to do so.
>>>>>
>>>>
>>>> I checked the current state, it seems to be the same.
>>>> Yes, the decision of postponing the actual hardware programming to the
>>>> point
>>>> where the interrupt actually gets used is right, but here above I was
>>>> talking
>>>> another thing.
>>>> As I mentioned, a message can associate with multiple interrupts. Enabling
>>>> any of them will call irq_startup(). So if we don't want to compose or
>>>> write
>>>> messages repeatedly, we'd better require performing some checks before
>>>> activating the interrupts.
>>> Hi Yun,
>>> Seems you are talking about the case of multiple MSI support.
>>> Yes, we have special treatment for multiple MSI, which only writes PCI
>>> MSI registers when starting up the first MSI interrupt.
>>> void pci_msi_domain_write_msg(struct irq_data *irq_data, struct msi_msg
>>> *msg)
>>> {
>>> struct msi_desc *desc = irq_data->msi_desc;
>>>
>>> /*
>>> * For MSI-X desc->irq is always equal to irq_data->irq. For
>>> * MSI only the first interrupt of MULTI MSI passes the test.
>>> */
>>> if (desc->irq == irq_data->irq)
>>> __pci_write_msi_msg(desc, msg);
>>> }
>>
>>
>>
Re: [patch 08/16] genirq: Introduce callback irq_chip.irq_write_msi_msg
On 2014/11/18 18:19, Thomas Gleixner wrote:
> On Tue, 18 Nov 2014, Yun Wu (Abel) wrote:
>> On 2014/11/12 21:43, Thomas Gleixner wrote:
>>> struct irq_chip {
>>> @@ -359,6 +360,7 @@ struct irq_chip {
>>> void(*irq_release_resources)(struct irq_data *data);
>>>
>>> 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);
>>
>> Hmm... It's really weird.
>> I don't think it's the interrupt controllers' responsibility to write
>> messages
>> for all the endpoint devices since the methods of configuring message
>> registers
>> may different between these devices. And theoretically, the endpoint devices
>> themselves should take the responsibility to configure their message
>> registers.
>> To say the least, the write_msg callback here still need to call some certain
>> interfaces provided by the corresponding device.
>>
>> There would be lots of ARM new devices capable of sending message
>> based interrupts to interrupt controllers, does all the drivers of
>> the devices need to expose a write_msg callback to interrupt
>> controllers?
>
> Well, writing the message _IS_ part of the interrupt controller.
>
> So in order to enable non PCI based MSI we want to have generic
> infrastructure with minimal per device/device class callbacks and of
> course you need to provide that callback for your special device.
>
> We already have non PCI based MSI controllers in x86 today and we need
> to handle the whole stuff with tons of copied coded extra for each of
> those. So consolidating it into common infrastructure allows us to get
> rid of the pointless copied code and reduce the per device effort to
> the relevant hardware specific callbacks. irq_write_msi_msg being one
> of those.
>
At least, we have the same goal.
I will illustrate my thoughts by an example.
The current code is something like:
Device A
void A_write_msg() { ... }
Group B
(a group of devices behave same on writing messages, i.e. PCI)
===
void B_write_msg() { ... }
Controller
==
irq_chip.irq_write_msi_msg () {
if (A)
A_write_msg();
if (B)
B_write_msg();
}
It's horrible when new devices come out, since we need to modify the
controller part for each new device.
What I suggested is:
MSI Core
struct msi_ops { .write_msg, };
struct msi_desc { .msi_ops, };
write_msg() {
X = get_dev();
irq_chip.compose_msg(X);// IRQ chips' responsibility
X_msi_ops.write_msg(); // nothing to do with IRQ chips
}
Device A
void A_write_msg() { ... }
A_msi_ops.write_msg = A_write_msg;
Group B
===
void B_write_msg() { ... }
B_msi_ops.write_msg = B_write_msg;
Please correct me if I misunderstood anything.
Thanks,
Abel
--
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 07/16] genirq: Introduce helper irq_domain_set_info() to reduce duplicated code
On 2014/11/18 20:38, Jiang Liu wrote:
> On 2014/11/18 19:47, Yun Wu (Abel) wrote:
>> On 2014/11/18 18:03, Thomas Gleixner wrote:
>>
>>> On Tue, 18 Nov 2014, Yun Wu (Abel) wrote:
>>>
>>>> Hi Thomas, Jiang,
>>>> On 2014/11/12 21:43, Thomas Gleixner wrote:
>>>>
>>>>> From: Jiang Liu
>>>> [...]
>>>>> +void irq_domain_set_info(struct irq_domain *domain, unsigned int virq,
>>>>> + irq_hw_number_t hwirq, struct irq_chip *chip,
>>>>> + void *chip_data, irq_flow_handler_t handler,
>>>>> + void *handler_data, const char *handler_name)
>>>>> +{
>>>>> + irq_domain_set_hwirq_and_chip(domain, virq, hwirq, chip, chip_data);
>>>>> + __irq_set_handler(virq, handler, 0, handler_name);
>>>>> + irq_set_handler_data(virq, handler_data);
>>>>> +}
>>>>
>>>> When stacked domain enabled, there will be a semantic shift to the linux
>>>> interrupt
>>>> identifiers. The @virq now delivers much more than before.
>>>> More specifically, now we need both @virq and @domain, rather than only
>>>> @irq, to
>>>> determine which irq_data we want to configure. And once we configure @irq
>>>> without
>>>> providing the exact domain, it means we are configuring all the domains
>>>> related to
>>>> that @irq. So I think this routine just messed all things up.
>>>
>>> You can mess up anything by using an interface in the wrong way. Open
>>> coding will not make that harder.
>>>
>>
>> But what's the correct way to use this interface?
>
> It's to be used by interrupt controller drivers to implement
> hierarchy irqdomains.
>
Each time an interrupt domain calls this, the previous (@handler, @handler_name,
@handler_data) will be overrode. It's because the routines __irq_set_handler()
and irq_set_handler_data() only configure top level (without Marc's fix which is
not ideal). Is this what we really want to see?
Thanks,
Abel
--
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 04/16] genirq: Introduce irq_chip.irq_compose_msi_msg() to support stacked irqchip
On 2014/11/18 20:43, Jiang Liu wrote:
> On 2014/11/18 19:47, Yun Wu (Abel) wrote:
>> On 2014/11/18 18:02, Thomas Gleixner wrote:
>>
>>> On Tue, 18 Nov 2014, Yun Wu (Abel) wrote:
>>>> On 2014/11/12 21:42, Thomas Gleixner wrote:
>>>>> +int irq_chip_compose_msi_msg(struct irq_data *data, struct msi_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_msi_msg)
>>>>> + pos = data;
>>>>> + if (!pos)
>>>>> + return -ENOSYS;
>>>>> +
>>>>> + pos->chip->irq_compose_msi_msg(pos, msg);
>>>>> +
>>>>> + return 0;
>>>>> +}
>>>>
>>>> Adding message composing routine to struct irq_chip is OK to me, and it
>>>> should
>>>> be because it is interrupt controllers' duty to compose messages (so that
>>>> they
>>>> can parse the messages correctly without any pre-defined rules that
>>>> endpoint
>>>> devices absolutely need not to know).
>>>> However a problem comes out when deciding which parameters should be
>>>> passed to
>>>> this routine. A message can associate with multiple interrupts, which
>>>> makes me
>>>> think composing messages for each interrupt is not that appropriate. And we
>>>> can take a look at the new routine irq_chip_compose_msi_msg(). It is
>>>> called by
>>>> msi_domain_activate() which will be called by irq_domain_activate_irq() in
>>>> irq_startup() for each interrupt descriptor, result in composing a message
>>>> for
>>>> each interrupt, right? (Unless requiring a judge on the parameter @data
>>>> when
>>>> implementing the irq_compose_msi_msg() callback that only compose message
>>>> for
>>>> the first entry of that message. But I really don't like that...)
>>>
>>> No, that's not correct. You are looking at some random stale version
>>> of this. The current state of affairs is in
>>>
>>>git://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git irq/irqdomain
>>>
>>> See also https://lkml.org/lkml/2014/11/17/764
>>>
>>> In activate we write the message, which is the right point to do so.
>>>
>>
>> I checked the current state, it seems to be the same.
>> Yes, the decision of postponing the actual hardware programming to the point
>> where the interrupt actually gets used is right, but here above I was talking
>> another thing.
>> As I mentioned, a message can associate with multiple interrupts. Enabling
>> any of them will call irq_startup(). So if we don't want to compose or write
>> messages repeatedly, we'd better require performing some checks before
>> activating the interrupts.
> Hi Yun,
> Seems you are talking about the case of multiple MSI support.
> Yes, we have special treatment for multiple MSI, which only writes PCI
> MSI registers when starting up the first MSI interrupt.
> void pci_msi_domain_write_msg(struct irq_data *irq_data, struct msi_msg
> *msg)
> {
> struct msi_desc *desc = irq_data->msi_desc;
>
> /*
> * For MSI-X desc->irq is always equal to irq_data->irq. For
> * MSI only the first interrupt of MULTI MSI passes the test.
> */
> if (desc->irq == irq_data->irq)
> __pci_write_msi_msg(desc, msg);
> }
Yes, I picked the case of multiple MSI support.
The check should also be performed when composing messages. That's why
I don't like its parameters. The @data only indicates one interrupt,
while I prefer doing compose/write in the unit of message descriptor.
Thanks,
Abel
--
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 09/16] genirq: Add generic msi irq domain support
On 2014/11/12 21:43, Thomas Gleixner wrote:
> From: Jiang Liu
>
> Implement the basic functions for MSI interrupt support with
> hierarchical interrupt domains.
>
> [ tglx: Extracted and combined from several patches ]
>
> Signed-off-by: Jiang Liu
> Cc: Bjorn Helgaas
> Cc: Grant Likely
> Cc: Marc Zyngier
> Cc: Yingjoe Chen
> Cc: Yijing Wang
> Signed-off-by: Thomas Gleixner
> ---
> include/linux/msi.h | 35 +++
> kernel/irq/Kconfig | 10
> kernel/irq/Makefile |1
> kernel/irq/msi.c| 119
>
> 4 files changed, 165 insertions(+)
>
> Index: tip/include/linux/msi.h
> ===
> --- tip.orig/include/linux/msi.h
> +++ tip/include/linux/msi.h
> @@ -77,4 +77,39 @@ struct msi_chip {
> void (*teardown_irq)(struct msi_chip *chip, unsigned int irq);
> };
>
> +#ifdef CONFIG_GENERIC_MSI_IRQ_DOMAIN
> +struct irq_domain;
> +struct irq_chip;
> +struct device_node;
> +struct msi_domain_info;
> +
> +struct msi_domain_ops {
> + void(*calc_hwirq)(struct msi_domain_info *info, void *arg,
> + struct msi_desc *desc);
> + irq_hw_number_t (*get_hwirq)(struct msi_domain_info *info, void *arg);
> + int (*msi_init)(struct irq_domain *domain,
> + struct msi_domain_info *info,
> + unsigned int virq, irq_hw_number_t hwirq,
> + void *arg);
> + void(*msi_free)(struct irq_domain *domain,
> + struct msi_domain_info *info,
> + unsigned int virq);
> +};
> +
> +struct msi_domain_info {
> + struct msi_domain_ops *ops;
> + struct irq_chip *chip;
> + void*data;
> +};
> +
> +int msi_domain_set_affinity(struct irq_data *data, const struct cpumask
> *mask,
> + bool force);
> +
> +struct irq_domain *msi_create_irq_domain(struct device_node *of_node,
> + struct msi_domain_info *info,
> + struct irq_domain *parent);
> +struct msi_domain_info *msi_get_domain_info(struct irq_domain *domain);
> +
> +#endif /* CONFIG_GENERIC_MSI_IRQ_DOMAIN */
> +
> #endif /* LINUX_MSI_H */
> Index: tip/kernel/irq/Kconfig
> ===
> --- tip.orig/kernel/irq/Kconfig
> +++ tip/kernel/irq/Kconfig
> @@ -60,6 +60,16 @@ config IRQ_DOMAIN_HIERARCHY
> bool
> select IRQ_DOMAIN
>
> +# Generic MSI interrupt support
> +config GENERIC_MSI_IRQ
> + bool
> +
> +# Generic MSI hierarchical interrupt domain support
> +config GENERIC_MSI_IRQ_DOMAIN
> + bool
> + select IRQ_DOMAIN_HIERARCHY
> + select GENERIC_MSI_IRQ
> +
> config HANDLE_DOMAIN_IRQ
> bool
>
> Index: tip/kernel/irq/Makefile
> ===
> --- tip.orig/kernel/irq/Makefile
> +++ tip/kernel/irq/Makefile
> @@ -6,3 +6,4 @@ obj-$(CONFIG_IRQ_DOMAIN) += irqdomain.o
> obj-$(CONFIG_PROC_FS) += proc.o
> obj-$(CONFIG_GENERIC_PENDING_IRQ) += migration.o
> obj-$(CONFIG_PM_SLEEP) += pm.o
> +obj-$(CONFIG_GENERIC_MSI_IRQ) += msi.o
> Index: tip/kernel/irq/msi.c
> ===
> --- /dev/null
> +++ tip/kernel/irq/msi.c
> @@ -0,0 +1,119 @@
> +/*
> + * linux/kernel/irq/msi.c
> + *
> + * Copyright (C) 2014 Intel Corp.
> + * Author: Jiang Liu
> + *
> + * This file is licensed under GPLv2.
> + *
> + * This file contains common code to support Message Signalled Interrupt for
> + * PCI compatible and non PCI compatible devices.
> + */
> +#include
> +#include
> +#include
> +
> +#ifdef CONFIG_GENERIC_MSI_IRQ_DOMAIN
> +int msi_domain_set_affinity(struct irq_data *irq_data,
> + const struct cpumask *mask, bool force)
> +{
> + struct irq_data *parent = irq_data->parent_data;
> + struct msi_msg msg;
> + int ret;
> +
> + ret = parent->chip->irq_set_affinity(parent, mask, force);
> + if (ret >= 0 && ret != IRQ_SET_MASK_OK_DONE) {
> + BUG_ON(irq_chip_compose_msi_msg(irq_data, ));
> + irq_chip_write_msi_msg(irq_data, );
> + }
> +
> + return ret;
> +}
> +
> +static void msi_domain_activate(struct irq_domain *domain,
> + struct irq_data *irq_data)
> +{
> + struct msi_msg msg;
> +
> + BUG_ON(irq_chip_compose_msi_msg(irq_data, ));
> + irq_chip_write_msi_msg(irq_data, );
> +}
> +
> +static void msi_domain_deactivate(struct irq_domain *domain,
> + struct irq_data *irq_data)
> +{
> + struct msi_msg msg;
> +
> + memset(, 0, sizeof(msg));
> + irq_chip_write_msi_msg(irq_data, );
> +}
> +
> +static int msi_domain_alloc(struct irq_domain *domain, unsigned int virq,
Re: [patch 01/16] irqdomain: Introduce new interfaces to support hierarchy irqdomains
On 2014/11/18 17:54, Thomas Gleixner wrote: > On Tue, 18 Nov 2014, Yun Wu (Abel) wrote: >> Hi Thomas, Jiang, >> On 2014/11/12 21:42, Thomas Gleixner wrote: >> >>> From: Jiang Liu >>> Index: tip/kernel/irq/chip.c >>> === >>> --- tip.orig/kernel/irq/chip.c >>> +++ tip/kernel/irq/chip.c >>> @@ -15,6 +15,7 @@ >>> #include >>> #include >>> #include >>> +#include >>> >>> #include >>> >>> @@ -178,6 +179,7 @@ int irq_startup(struct irq_desc *desc, b >>> irq_state_clr_disabled(desc); >>> desc->depth = 0; >>> >>> + irq_domain_activate_irq(>irq_data); >> >> I'm not sure why this is needed, please help me out.. :) > > Because it makes the whole error handling in stacked allocations way > simpler. > > We allocate and reserve resources, but do not program the hardware up > to the point where request_irq and therefor irq_startup() is invoked. > > So when in the allocation/reservation phase any of the stack level > fails we just have to undo the allocations/reservations and not any > hardware settings. > > That also solves the issue that depending on the stacking we might not > be able to program the hardware during the allocation because all > stack levels need to be allocated/reserved before we can figure out > which hardware configuration we need for the various levels. > > So we decided to postpone the actual hardware programming to the point > where the interrupt actually gets used. > OK, I got it. Thanks, Abel -- 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 07/16] genirq: Introduce helper irq_domain_set_info() to reduce duplicated code
On 2014/11/18 18:03, Thomas Gleixner wrote:
> On Tue, 18 Nov 2014, Yun Wu (Abel) wrote:
>
>> Hi Thomas, Jiang,
>> On 2014/11/12 21:43, Thomas Gleixner wrote:
>>
>>> From: Jiang Liu
>> [...]
>>> +void irq_domain_set_info(struct irq_domain *domain, unsigned int virq,
>>> +irq_hw_number_t hwirq, struct irq_chip *chip,
>>> +void *chip_data, irq_flow_handler_t handler,
>>> +void *handler_data, const char *handler_name)
>>> +{
>>> + irq_domain_set_hwirq_and_chip(domain, virq, hwirq, chip, chip_data);
>>> + __irq_set_handler(virq, handler, 0, handler_name);
>>> + irq_set_handler_data(virq, handler_data);
>>> +}
>>
>> When stacked domain enabled, there will be a semantic shift to the linux
>> interrupt
>> identifiers. The @virq now delivers much more than before.
>> More specifically, now we need both @virq and @domain, rather than only
>> @irq, to
>> determine which irq_data we want to configure. And once we configure @irq
>> without
>> providing the exact domain, it means we are configuring all the domains
>> related to
>> that @irq. So I think this routine just messed all things up.
>
> You can mess up anything by using an interface in the wrong way. Open
> coding will not make that harder.
>
But what's the correct way to use this interface?
Thanks,
Abel
--
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 04/16] genirq: Introduce irq_chip.irq_compose_msi_msg() to support stacked irqchip
On 2014/11/18 18:02, Thomas Gleixner wrote:
> On Tue, 18 Nov 2014, Yun Wu (Abel) wrote:
>> On 2014/11/12 21:42, Thomas Gleixner wrote:
>>> +int irq_chip_compose_msi_msg(struct irq_data *data, struct msi_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_msi_msg)
>>> + pos = data;
>>> + if (!pos)
>>> + return -ENOSYS;
>>> +
>>> + pos->chip->irq_compose_msi_msg(pos, msg);
>>> +
>>> + return 0;
>>> +}
>>
>> Adding message composing routine to struct irq_chip is OK to me, and it
>> should
>> be because it is interrupt controllers' duty to compose messages (so that
>> they
>> can parse the messages correctly without any pre-defined rules that endpoint
>> devices absolutely need not to know).
>> However a problem comes out when deciding which parameters should be passed
>> to
>> this routine. A message can associate with multiple interrupts, which makes
>> me
>> think composing messages for each interrupt is not that appropriate. And we
>> can take a look at the new routine irq_chip_compose_msi_msg(). It is called
>> by
>> msi_domain_activate() which will be called by irq_domain_activate_irq() in
>> irq_startup() for each interrupt descriptor, result in composing a message
>> for
>> each interrupt, right? (Unless requiring a judge on the parameter @data when
>> implementing the irq_compose_msi_msg() callback that only compose message for
>> the first entry of that message. But I really don't like that...)
>
> No, that's not correct. You are looking at some random stale version
> of this. The current state of affairs is in
>
>git://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git irq/irqdomain
>
> See also https://lkml.org/lkml/2014/11/17/764
>
> In activate we write the message, which is the right point to do so.
>
I checked the current state, it seems to be the same.
Yes, the decision of postponing the actual hardware programming to the point
where the interrupt actually gets used is right, but here above I was talking
another thing.
As I mentioned, a message can associate with multiple interrupts. Enabling
any of them will call irq_startup(). So if we don't want to compose or write
messages repeatedly, we'd better require performing some checks before
activating the interrupts.
Thanks,
Abel
--
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 07/16] genirq: Introduce helper irq_domain_set_info() to reduce duplicated code
Hi Thomas, Jiang,
On 2014/11/12 21:43, Thomas Gleixner wrote:
> From: Jiang Liu
[...]
> +void irq_domain_set_info(struct irq_domain *domain, unsigned int virq,
> + irq_hw_number_t hwirq, struct irq_chip *chip,
> + void *chip_data, irq_flow_handler_t handler,
> + void *handler_data, const char *handler_name)
> +{
> + irq_domain_set_hwirq_and_chip(domain, virq, hwirq, chip, chip_data);
> + __irq_set_handler(virq, handler, 0, handler_name);
> + irq_set_handler_data(virq, handler_data);
> +}
When stacked domain enabled, there will be a semantic shift to the linux
interrupt
identifiers. The @virq now delivers much more than before.
More specifically, now we need both @virq and @domain, rather than only @irq, to
determine which irq_data we want to configure. And once we configure @irq
without
providing the exact domain, it means we are configuring all the domains related
to
that @irq. So I think this routine just messed all things up.
Regards,
Abel
--
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 08/16] genirq: Introduce callback irq_chip.irq_write_msi_msg
Hi Thomas, Jiang,
On 2014/11/12 21:43, Thomas Gleixner wrote:
> From: Jiang Liu
>
> Introduce callback irq_chip.irq_write_msi_msg, so we can share common
> code among MSI alike interrupt controllers, such as HPET and DMAR.
>
> Signed-off-by: Jiang Liu
> Cc: Bjorn Helgaas
> Cc: Grant Likely
> Cc: Marc Zyngier
> Cc: Yingjoe Chen
> Cc: Yijing Wang
> Signed-off-by: Thomas Gleixner
> ---
> include/linux/irq.h |8
> 1 file changed, 8 insertions(+)
>
> Index: tip/include/linux/irq.h
> ===
> --- tip.orig/include/linux/irq.h
> +++ tip/include/linux/irq.h
> @@ -322,6 +322,7 @@ static inline irq_hw_number_t irqd_to_hw
> * @irq_release_resources: optional to release resources acquired with
> * irq_request_resources
> * @irq_compose_msi_msg: optional to compose message content for MSI
> + * @irq_write_msi_msg: optional to write message content for MSI
> * @flags: chip specific flags
> */
> struct irq_chip {
> @@ -359,6 +360,7 @@ struct irq_chip {
> void(*irq_release_resources)(struct irq_data *data);
>
> 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);
Hmm... It's really weird.
I don't think it's the interrupt controllers' responsibility to write messages
for all the endpoint devices since the methods of configuring message registers
may different between these devices. And theoretically, the endpoint devices
themselves should take the responsibility to configure their message registers.
To say the least, the write_msg callback here still need to call some certain
interfaces provided by the corresponding device.
There would be lots of ARM new devices capable of sending message based
interrupts
to interrupt controllers, does all the drivers of the devices need to expose a
write_msg callback to interrupt controllers?
Regards,
Abel
>
> unsigned long flags;
> };
> @@ -453,6 +455,12 @@ extern void irq_chip_ack_parent(struct i
> extern int irq_chip_retrigger_hierarchy(struct irq_data *data);
> #endif
>
> +static inline void irq_chip_write_msi_msg(struct irq_data *data,
> + struct msi_msg *msg)
> +{
> + data->chip->irq_write_msi_msg(data, msg);
> +}
> +
> /* Handling of unhandled and spurious interrupts: */
> extern void note_interrupt(unsigned int irq, struct irq_desc *desc,
> irqreturn_t action_ret);
>
>
> --
> 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 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 04/16] genirq: Introduce irq_chip.irq_compose_msi_msg() to support stacked irqchip
On 2014/11/12 21:42, Thomas Gleixner wrote:
> From: Jiang Liu
>
> Add callback irq_compose_msi_msg to struct irq_chip, which will be used
> to support stacked irqchip.
>
> Signed-off-by: Jiang Liu
> Cc: Bjorn Helgaas
> Cc: Grant Likely
> Cc: Marc Zyngier
> Cc: Yingjoe Chen
> Cc: Yijing Wang
> Signed-off-by: Thomas Gleixner
> ---
> include/linux/irq.h |5 +
> kernel/irq/chip.c | 17 +
> 2 files changed, 22 insertions(+)
>
> diff --git a/include/linux/irq.h b/include/linux/irq.h
> index 0adcbbbf2e87..536b7fc6c8f4 100644
> --- a/include/linux/irq.h
> +++ b/include/linux/irq.h
> @@ -29,6 +29,7 @@ struct seq_file;
> struct module;
> struct irq_desc;
> struct irq_data;
> +struct msi_msg;
> typedef void (*irq_flow_handler_t)(unsigned int irq,
> struct irq_desc *desc);
> typedef void (*irq_preflow_handler_t)(struct irq_data *data);
> @@ -320,6 +321,7 @@ static inline irq_hw_number_t irqd_to_hwirq(struct
> irq_data *d)
> * any other callback related to this irq
> * @irq_release_resources: optional to release resources acquired with
> * irq_request_resources
> + * @irq_compose_msi_msg: optional to compose message content for MSI
> * @flags: chip specific flags
> */
> struct irq_chip {
> @@ -356,6 +358,8 @@ struct irq_chip {
> int (*irq_request_resources)(struct irq_data *data);
> void(*irq_release_resources)(struct irq_data *data);
>
> + void(*irq_compose_msi_msg)(struct irq_data *data, struct
> msi_msg *msg);
> +
> unsigned long flags;
> };
>
> @@ -443,6 +447,7 @@ extern void handle_percpu_devid_irq(unsigned int irq,
> struct irq_desc *desc);
> extern void handle_bad_irq(unsigned int irq, struct irq_desc *desc);
> extern void handle_nested_irq(unsigned int irq);
>
> +extern int irq_chip_compose_msi_msg(struct irq_data *data, struct msi_msg
> *msg);
> #ifdef CONFIG_IRQ_DOMAIN_HIERARCHY
> extern void irq_chip_ack_parent(struct irq_data *data);
> extern int irq_chip_retrigger_hierarchy(struct irq_data *data);
> diff --git a/kernel/irq/chip.c b/kernel/irq/chip.c
> index 12f3e72449eb..8f362db17a8a 100644
> --- a/kernel/irq/chip.c
> +++ b/kernel/irq/chip.c
> @@ -867,3 +867,20 @@ int irq_chip_retrigger_hierarchy(struct irq_data *data)
> return -ENOSYS;
> }
> #endif
> +
> +int irq_chip_compose_msi_msg(struct irq_data *data, struct msi_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_msi_msg)
> + pos = data;
> + if (!pos)
> + return -ENOSYS;
> +
> + pos->chip->irq_compose_msi_msg(pos, msg);
> +
> + return 0;
> +}
Adding message composing routine to struct irq_chip is OK to me, and it should
be because it is interrupt controllers' duty to compose messages (so that they
can parse the messages correctly without any pre-defined rules that endpoint
devices absolutely need not to know).
However a problem comes out when deciding which parameters should be passed to
this routine. A message can associate with multiple interrupts, which makes me
think composing messages for each interrupt is not that appropriate. And we
can take a look at the new routine irq_chip_compose_msi_msg(). It is called by
msi_domain_activate() which will be called by irq_domain_activate_irq() in
irq_startup() for each interrupt descriptor, result in composing a message for
each interrupt, right? (Unless requiring a judge on the parameter @data when
implementing the irq_compose_msi_msg() callback that only compose message for
the first entry of that message. But I really don't like that...)
Regards,
Abel
--
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 01/16] irqdomain: Introduce new interfaces to support hierarchy irqdomains
Hi Thomas, Jiang,
On 2014/11/12 21:42, Thomas Gleixner wrote:
> From: Jiang Liu
> Index: tip/kernel/irq/chip.c
> ===
> --- tip.orig/kernel/irq/chip.c
> +++ tip/kernel/irq/chip.c
> @@ -15,6 +15,7 @@
> #include
> #include
> #include
> +#include
>
> #include
>
> @@ -178,6 +179,7 @@ int irq_startup(struct irq_desc *desc, b
> irq_state_clr_disabled(desc);
> desc->depth = 0;
>
> + irq_domain_activate_irq(>irq_data);
I'm not sure why this is needed, please help me out.. :)
Thanks,
Abel
> if (desc->irq_data.chip->irq_startup) {
> ret = desc->irq_data.chip->irq_startup(>irq_data);
> irq_state_clr_masked(desc);
> @@ -199,6 +201,7 @@ void irq_shutdown(struct irq_desc *desc)
> desc->irq_data.chip->irq_disable(>irq_data);
> else
> desc->irq_data.chip->irq_mask(>irq_data);
> + irq_domain_deactivate_irq(>irq_data);
> irq_state_set_masked(desc);
> }
>
--
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 00/16] genirq: Hierarchical irq domains and generic MSI interrupt code
Hi Thomas, Jiang, I finally get some time on this, hopefully not too late... In brief, I like the part of stacked domain, and have some opinions on the part of refactoring MSI. Please check inline comments. Regards, Abel On 2014/11/12 21:42, Thomas Gleixner wrote: > This is an extract from Jiangs various patch series which only > contains the generic irq and MSI parts w/o the x86 specific > modifications. > > This is roughly what I plan to merge into the generic irq core, so the > various outstanding patches (irqchip/x86/...) can be based on this. > > It's available from git as well: > > git://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git irq/irqdomain > > Note, that branch is not yet exposed to linux-next and subject to > modifications including rebasing. > > Can all involved parties please have a close look and retest their > patches on top of this? > > Jiang, you need to merge that into x86/apic for rebasing your series. > > Thanks, > > tglx > > Documentation/IRQ-domain.txt | 71 + > arch/arm/mach-iop13xx/msi.c|2 > arch/ia64/kernel/msi_ia64.c|4 > arch/ia64/sn/kernel/msi_sn.c |4 > arch/mips/pci/msi-octeon.c |2 > arch/mips/pci/msi-xlp.c|2 > arch/mips/pci/pci-xlr.c|2 > arch/powerpc/platforms/cell/axon_msi.c |2 > arch/powerpc/platforms/powernv/pci.c |2 > arch/powerpc/platforms/pseries/msi.c |2 > arch/powerpc/sysdev/fsl_msi.c |2 > arch/powerpc/sysdev/mpic_pasemi_msi.c |2 > arch/powerpc/sysdev/mpic_u3msi.c |2 > arch/powerpc/sysdev/ppc4xx_hsta_msi.c |2 > arch/powerpc/sysdev/ppc4xx_msi.c |2 > arch/s390/pci/pci.c|2 > arch/sparc/kernel/pci_msi.c|2 > arch/tile/kernel/pci_gx.c |2 > arch/x86/kernel/apic/io_apic.c |4 > arch/x86/pci/xen.c |4 > drivers/iommu/irq_remapping.c |8 > drivers/irqchip/irq-armada-370-xp.c|2 > drivers/pci/Kconfig|6 > drivers/pci/host/pci-tegra.c |2 > drivers/pci/host/pcie-designware.c |2 > drivers/pci/host/pcie-rcar.c |2 > drivers/pci/host/pcie-xilinx.c |2 > drivers/pci/msi.c | 150 ++- > drivers/vfio/pci/vfio_pci_intrs.c |2 > include/linux/irq.h| 33 ++ > include/linux/irqdomain.h | 91 +++ > include/linux/irqhandler.h | 14 + > include/linux/msi.h| 53 +++- > kernel/irq/Kconfig | 15 + > kernel/irq/Makefile|1 > kernel/irq/chip.c | 37 ++ > kernel/irq/irqdomain.c | 418 > +++-- > kernel/irq/manage.c|2 > kernel/irq/msi.c | 132 ++ > 39 files changed, 959 insertions(+), 130 deletions(-) > > > > > -- > 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 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 09/16] genirq: Add generic msi irq domain support
On 2014/11/18 22:24, Thomas Gleixner wrote:
On Tue, 18 Nov 2014, Yun Wu (Abel) wrote:
On 2014/11/18 20:49, Jiang Liu wrote:
+ ret = irq_domain_alloc_irqs_parent(domain, virq, nr_irqs, arg);
+ if (ret 0)
+ return ret;
+
+ for (i = 0; i nr_irqs; i++) {
+ ret = ops-msi_init(domain, info, virq + i, hwirq + i, arg);
+ if (ret 0) {
+ if (ops-msi_free) {
+ for (i--; i 0; i--)
while (i--) ?
The allocation for 'i' failed. So we don't free i. You missed the real
bug here:
+ for (i--; i 0; i--)
Must be i = 0.
for (i--; i = 0; i--) is just the same as while (i--).
Thanks,
Abel
--
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 08/16] genirq: Introduce callback irq_chip.irq_write_msi_msg
On 2014/11/18 22:19, Thomas Gleixner wrote: On Tue, 18 Nov 2014, Yun Wu (Abel) wrote: On 2014/11/18 21:43, Jiang Liu wrote: We provide an irq_chip for each type of interrupt controller instead of devices. For the example mentioned above, if device A and Group B has different interrupt controllers, we just need to implement irq_chip_A and irq_chip_B and set irq_chip.irq_write_msi_msg() to suitable callbacks. The framework already achieves what you you want:) What if device A and group B have the same interrupt controller? Well, if write_msg() is different they are hardly the same. The GICv3 ITS now deals with both PCI and non PCI message interrupts. We can't require the new devices behave writing message in a same way. What we can do is to abstract all the endpoints' behavior, and I provided one abstraction in an earlier reply. Thanks, Abel -- 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 08/16] genirq: Introduce callback irq_chip.irq_write_msi_msg
On 2014/11/18 22:29, Jiang Liu wrote:
On 2014/11/18 22:22, Yun Wu (Abel) wrote:
On 2014/11/18 22:03, Jiang Liu wrote:
On 2014/11/18 21:52, Yun Wu (Abel) wrote:
On 2014/11/18 21:43, Jiang Liu wrote:
On 2014/11/18 21:33, Yun Wu (Abel) wrote:
On 2014/11/18 18:19, Thomas Gleixner wrote:
On Tue, 18 Nov 2014, Yun Wu (Abel) wrote:
On 2014/11/12 21:43, Thomas Gleixner wrote:
struct irq_chip {
@@ -359,6 +360,7 @@ struct irq_chip {
void(*irq_release_resources)(struct irq_data *data);
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);
Hmm... It's really weird.
I don't think it's the interrupt controllers' responsibility to write
messages
for all the endpoint devices since the methods of configuring message
registers
may different between these devices. And theoretically, the endpoint
devices
themselves should take the responsibility to configure their message
registers.
To say the least, the write_msg callback here still need to call some
certain
interfaces provided by the corresponding device.
There would be lots of ARM new devices capable of sending message
based interrupts to interrupt controllers, does all the drivers of
the devices need to expose a write_msg callback to interrupt
controllers?
Well, writing the message _IS_ part of the interrupt controller.
So in order to enable non PCI based MSI we want to have generic
infrastructure with minimal per device/device class callbacks and of
course you need to provide that callback for your special device.
We already have non PCI based MSI controllers in x86 today and we need
to handle the whole stuff with tons of copied coded extra for each of
those. So consolidating it into common infrastructure allows us to get
rid of the pointless copied code and reduce the per device effort to
the relevant hardware specific callbacks. irq_write_msi_msg being one
of those.
At least, we have the same goal.
I will illustrate my thoughts by an example.
The current code is something like:
Device A
void A_write_msg() { ... }
Group B
(a group of devices behave same on writing messages, i.e. PCI)
===
void B_write_msg() { ... }
Controller
==
irq_chip.irq_write_msi_msg () {
if (A)
A_write_msg();
if (B)
B_write_msg();
}
It's horrible when new devices come out, since we need to modify the
controller part for each new device.
What I suggested is:
MSI Core
struct msi_ops { .write_msg, };
struct msi_desc { .msi_ops, };
write_msg() {
X = get_dev();
irq_chip.compose_msg(X);// IRQ chips' responsibility
X_msi_ops.write_msg(); // nothing to do with IRQ chips
}
Device A
void A_write_msg() { ... }
A_msi_ops.write_msg = A_write_msg;
Group B
===
void B_write_msg() { ... }
B_msi_ops.write_msg = B_write_msg;
Please correct me if I misunderstood anything.
Hi Yun,
We provide an irq_chip for each type of interrupt controller
instead of devices. For the example mentioned above, if device A
and Group B has different interrupt controllers, we just need to
implement irq_chip_A and irq_chip_B and set irq_chip.irq_write_msi_msg()
to suitable callbacks.
The framework already achieves what you you want:)
What if device A and group B have the same interrupt controller?
Device doesn't care about interrupt controllers, it just cares about
interrupts used by itself. It's the interrupt source (controller)
enumerators' responsibility to discover interrupt source, associate
methods to control the interrupt source and assign irq numbers for
interrupt sources.
Yes, indeed.
There are two ways to associate irq numbers with device:
1) arch code/bus drivers statically assigns irq number for devices
when creating device objects, such as PCI legacy interrupt
(INTA-INTD), IOAPIC interrupts.
And OF interfaces.
2) device drivers ask interrupt source enumerators to dynamically
create irq numbers, such pci_enable_msix_range() and friends.
So device driver definitely doesn't need to known about irq_chip
methods to control interrupt sources.
The above you described is absolutely right, but not the things I want
to know. :)
Take GICv3 ITS for example, it deals with both PCI and non PCI message
interrupts. IIUC, several irq_chips need to be implemented in the ITS
driver (i.e. pci_msi_chip, A_msi_chip and B_msi_chip). What should we
do to the ITS driver if new MSI-capable devices come out?
Marc has posted a patchset to enable ITS based on the hierarchy
irqdomain framework, please refer to [PATCH 00/15] arm64: PCI/MSI:
GICv3 ITS support (stacked domain edition) at
https://lkml.org/lkml/2014/11/11/620
IIUC, Marc's patch now only supports PCI MSI/MSI-X...
Thanks,
Abel
--
To unsubscribe from this list: send the line unsubscribe linux-kernel
Re: [patch 08/16] genirq: Introduce callback irq_chip.irq_write_msi_msg
On 2014/11/19 1:14, Marc Zyngier wrote: On Tue, Nov 18 2014 at 2:46:02 pm GMT, Yun Wu (Abel) wuyun...@huawei.com wrote: [...] IIUC, Marc's patch now only supports PCI MSI/MSI-X... Indeed, and the current solution makes is relatively easy to plug in non-PCI MSI. Just don't plug the ITS into the *PCI* MSI framework when you encounter such a thing. I am looking forward to that. :) I guess the main reason you haven't plugged in non-PCI MSI is the way of obtaining device ids used in searching DT table. Thanks, Abel -- 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 08/16] genirq: Introduce callback irq_chip.irq_write_msi_msg
On 2014/11/19 1:21, Marc Zyngier wrote: On Tue, Nov 18 2014 at 2:34:44 pm GMT, Yun Wu (Abel) wuyun...@huawei.com wrote: On 2014/11/18 22:19, Thomas Gleixner wrote: On Tue, 18 Nov 2014, Yun Wu (Abel) wrote: On 2014/11/18 21:43, Jiang Liu wrote: We provide an irq_chip for each type of interrupt controller instead of devices. For the example mentioned above, if device A and Group B has different interrupt controllers, we just need to implement irq_chip_A and irq_chip_B and set irq_chip.irq_write_msi_msg() to suitable callbacks. The framework already achieves what you you want:) What if device A and group B have the same interrupt controller? Well, if write_msg() is different they are hardly the same. The GICv3 ITS now deals with both PCI and non PCI message interrupts. We can't require the new devices behave writing message in a same way. What we can do is to abstract all the endpoints' behavior, and I provided one abstraction in an earlier reply. This is why the framework gives you the opportunity to provide methods that: - compose the message - program the message into the device None of that has to be PCI specific, and gives you a clean abstraction. The framework only gives you a number of shortcuts for PCI MSI, because that's what most people care about. Indeed, and I never said Jiang's patches don't work, I was just thinking that they were not that perfect. Thanks, Abel -- 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 08/16] genirq: Introduce callback irq_chip.irq_write_msi_msg
On 2014/11/18 22:52, Jiang Liu wrote: On 2014/11/18 22:34, Yun Wu (Abel) wrote: On 2014/11/18 22:19, Thomas Gleixner wrote: On Tue, 18 Nov 2014, Yun Wu (Abel) wrote: On 2014/11/18 21:43, Jiang Liu wrote: We provide an irq_chip for each type of interrupt controller instead of devices. For the example mentioned above, if device A and Group B has different interrupt controllers, we just need to implement irq_chip_A and irq_chip_B and set irq_chip.irq_write_msi_msg() to suitable callbacks. The framework already achieves what you you want:) What if device A and group B have the same interrupt controller? Well, if write_msg() is different they are hardly the same. The GICv3 ITS now deals with both PCI and non PCI message interrupts. We can't require the new devices behave writing message in a same way. What we can do is to abstract all the endpoints' behavior, and I provided one abstraction in an earlier reply. It should be easy to extend:) Actually, x86 interrupt remapping drivers already support two types of MSIs, one is PCI MSI/MSIX, another is HPET interrupt. Well, if there are one hundred types, I don't think it's as easy as you thought to extend. Of course we can doubt the possibility of being hundred, but tens or twenties is reasonably possible lying under the fact we have already startet to integrate the MSI registers (or some other form to store information) into the individual devices. Thanks, Abel -- 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 08/16] genirq: Introduce callback irq_chip.irq_write_msi_msg
On 2014/11/18 22:32, Thomas Gleixner wrote: On Tue, 18 Nov 2014, Yun Wu (Abel) wrote: Can you please trim the messages when you're replying? The above you described is absolutely right, but not the things I want to know. :) Take GICv3 ITS for example, it deals with both PCI and non PCI message interrupts. IIUC, several irq_chips need to be implemented in the ITS driver (i.e. pci_msi_chip, A_msi_chip and B_msi_chip). What should we do to the ITS driver if new MSI-capable devices come out? You seem to miss the stacking here PCI-MSI - A-MSI - ITS - GIC B-MSI - So each of the device types has its own MSI controller. Each of them will have their own callbacks and are backed by the underlying ITS/GIC implementation. Yes, this hits the key point. Once a new device type becomes available, we need to add pieces of code outside the new device's driver to make it work, which in my opinion is due to lack of core infrastructure. More specifically, the core infrastructure needs to support mechanism of MSI, not the various types of devices. And that's the only sensible solution. It's sensible, but not perfect. What I suggested is to add a generic layer: PCI-MSI - A-MSI - (generic layer) - ITS - GICR B-MSI - The PCI/A/B/... passes its hardware properties to the generic layer which gets configurations ready by calling ITS's domain/chip callbacks. When a new device type arrives, the only thing need to do is to implement the driver of that device, with nothing to do with the generic layer or ITS. Thanks, Abel -- 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 00/16] genirq: Hierarchical irq domains and generic MSI interrupt code
Hi Thomas, Jiang, I finally get some time on this, hopefully not too late... In brief, I like the part of stacked domain, and have some opinions on the part of refactoring MSI. Please check inline comments. Regards, Abel On 2014/11/12 21:42, Thomas Gleixner wrote: This is an extract from Jiangs various patch series which only contains the generic irq and MSI parts w/o the x86 specific modifications. This is roughly what I plan to merge into the generic irq core, so the various outstanding patches (irqchip/x86/...) can be based on this. It's available from git as well: git://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git irq/irqdomain Note, that branch is not yet exposed to linux-next and subject to modifications including rebasing. Can all involved parties please have a close look and retest their patches on top of this? Jiang, you need to merge that into x86/apic for rebasing your series. Thanks, tglx Documentation/IRQ-domain.txt | 71 + arch/arm/mach-iop13xx/msi.c|2 arch/ia64/kernel/msi_ia64.c|4 arch/ia64/sn/kernel/msi_sn.c |4 arch/mips/pci/msi-octeon.c |2 arch/mips/pci/msi-xlp.c|2 arch/mips/pci/pci-xlr.c|2 arch/powerpc/platforms/cell/axon_msi.c |2 arch/powerpc/platforms/powernv/pci.c |2 arch/powerpc/platforms/pseries/msi.c |2 arch/powerpc/sysdev/fsl_msi.c |2 arch/powerpc/sysdev/mpic_pasemi_msi.c |2 arch/powerpc/sysdev/mpic_u3msi.c |2 arch/powerpc/sysdev/ppc4xx_hsta_msi.c |2 arch/powerpc/sysdev/ppc4xx_msi.c |2 arch/s390/pci/pci.c|2 arch/sparc/kernel/pci_msi.c|2 arch/tile/kernel/pci_gx.c |2 arch/x86/kernel/apic/io_apic.c |4 arch/x86/pci/xen.c |4 drivers/iommu/irq_remapping.c |8 drivers/irqchip/irq-armada-370-xp.c|2 drivers/pci/Kconfig|6 drivers/pci/host/pci-tegra.c |2 drivers/pci/host/pcie-designware.c |2 drivers/pci/host/pcie-rcar.c |2 drivers/pci/host/pcie-xilinx.c |2 drivers/pci/msi.c | 150 ++- drivers/vfio/pci/vfio_pci_intrs.c |2 include/linux/irq.h| 33 ++ include/linux/irqdomain.h | 91 +++ include/linux/irqhandler.h | 14 + include/linux/msi.h| 53 +++- kernel/irq/Kconfig | 15 + kernel/irq/Makefile|1 kernel/irq/chip.c | 37 ++ kernel/irq/irqdomain.c | 418 +++-- kernel/irq/manage.c|2 kernel/irq/msi.c | 132 ++ 39 files changed, 959 insertions(+), 130 deletions(-) -- 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 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 01/16] irqdomain: Introduce new interfaces to support hierarchy irqdomains
Hi Thomas, Jiang,
On 2014/11/12 21:42, Thomas Gleixner wrote:
From: Jiang Liu jiang@linux.intel.com
Index: tip/kernel/irq/chip.c
===
--- tip.orig/kernel/irq/chip.c
+++ tip/kernel/irq/chip.c
@@ -15,6 +15,7 @@
#include linux/module.h
#include linux/interrupt.h
#include linux/kernel_stat.h
+#include linux/irqdomain.h
#include trace/events/irq.h
@@ -178,6 +179,7 @@ int irq_startup(struct irq_desc *desc, b
irq_state_clr_disabled(desc);
desc-depth = 0;
+ irq_domain_activate_irq(desc-irq_data);
I'm not sure why this is needed, please help me out.. :)
Thanks,
Abel
if (desc-irq_data.chip-irq_startup) {
ret = desc-irq_data.chip-irq_startup(desc-irq_data);
irq_state_clr_masked(desc);
@@ -199,6 +201,7 @@ void irq_shutdown(struct irq_desc *desc)
desc-irq_data.chip-irq_disable(desc-irq_data);
else
desc-irq_data.chip-irq_mask(desc-irq_data);
+ irq_domain_deactivate_irq(desc-irq_data);
irq_state_set_masked(desc);
}
--
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 08/16] genirq: Introduce callback irq_chip.irq_write_msi_msg
Hi Thomas, Jiang,
On 2014/11/12 21:43, Thomas Gleixner wrote:
From: Jiang Liu jiang@linux.intel.com
Introduce callback irq_chip.irq_write_msi_msg, so we can share common
code among MSI alike interrupt controllers, such as HPET and DMAR.
Signed-off-by: Jiang Liu jiang@linux.intel.com
Cc: Bjorn Helgaas bhelg...@google.com
Cc: Grant Likely grant.lik...@linaro.org
Cc: Marc Zyngier marc.zyng...@arm.com
Cc: Yingjoe Chen yingjoe.c...@mediatek.com
Cc: Yijing Wang wangyij...@huawei.com
Signed-off-by: Thomas Gleixner t...@linutronix.de
---
include/linux/irq.h |8
1 file changed, 8 insertions(+)
Index: tip/include/linux/irq.h
===
--- tip.orig/include/linux/irq.h
+++ tip/include/linux/irq.h
@@ -322,6 +322,7 @@ static inline irq_hw_number_t irqd_to_hw
* @irq_release_resources: optional to release resources acquired with
* irq_request_resources
* @irq_compose_msi_msg: optional to compose message content for MSI
+ * @irq_write_msi_msg: optional to write message content for MSI
* @flags: chip specific flags
*/
struct irq_chip {
@@ -359,6 +360,7 @@ struct irq_chip {
void(*irq_release_resources)(struct irq_data *data);
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);
Hmm... It's really weird.
I don't think it's the interrupt controllers' responsibility to write messages
for all the endpoint devices since the methods of configuring message registers
may different between these devices. And theoretically, the endpoint devices
themselves should take the responsibility to configure their message registers.
To say the least, the write_msg callback here still need to call some certain
interfaces provided by the corresponding device.
There would be lots of ARM new devices capable of sending message based
interrupts
to interrupt controllers, does all the drivers of the devices need to expose a
write_msg callback to interrupt controllers?
Regards,
Abel
unsigned long flags;
};
@@ -453,6 +455,12 @@ extern void irq_chip_ack_parent(struct i
extern int irq_chip_retrigger_hierarchy(struct irq_data *data);
#endif
+static inline void irq_chip_write_msi_msg(struct irq_data *data,
+ struct msi_msg *msg)
+{
+ data-chip-irq_write_msi_msg(data, msg);
+}
+
/* Handling of unhandled and spurious interrupts: */
extern void note_interrupt(unsigned int irq, struct irq_desc *desc,
irqreturn_t action_ret);
--
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 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 04/16] genirq: Introduce irq_chip.irq_compose_msi_msg() to support stacked irqchip
On 2014/11/12 21:42, Thomas Gleixner wrote:
From: Jiang Liu jiang@linux.intel.com
Add callback irq_compose_msi_msg to struct irq_chip, which will be used
to support stacked irqchip.
Signed-off-by: Jiang Liu jiang@linux.intel.com
Cc: Bjorn Helgaas bhelg...@google.com
Cc: Grant Likely grant.lik...@linaro.org
Cc: Marc Zyngier marc.zyng...@arm.com
Cc: Yingjoe Chen yingjoe.c...@mediatek.com
Cc: Yijing Wang wangyij...@huawei.com
Signed-off-by: Thomas Gleixner t...@linutronix.de
---
include/linux/irq.h |5 +
kernel/irq/chip.c | 17 +
2 files changed, 22 insertions(+)
diff --git a/include/linux/irq.h b/include/linux/irq.h
index 0adcbbbf2e87..536b7fc6c8f4 100644
--- a/include/linux/irq.h
+++ b/include/linux/irq.h
@@ -29,6 +29,7 @@ struct seq_file;
struct module;
struct irq_desc;
struct irq_data;
+struct msi_msg;
typedef void (*irq_flow_handler_t)(unsigned int irq,
struct irq_desc *desc);
typedef void (*irq_preflow_handler_t)(struct irq_data *data);
@@ -320,6 +321,7 @@ static inline irq_hw_number_t irqd_to_hwirq(struct
irq_data *d)
* any other callback related to this irq
* @irq_release_resources: optional to release resources acquired with
* irq_request_resources
+ * @irq_compose_msi_msg: optional to compose message content for MSI
* @flags: chip specific flags
*/
struct irq_chip {
@@ -356,6 +358,8 @@ struct irq_chip {
int (*irq_request_resources)(struct irq_data *data);
void(*irq_release_resources)(struct irq_data *data);
+ void(*irq_compose_msi_msg)(struct irq_data *data, struct
msi_msg *msg);
+
unsigned long flags;
};
@@ -443,6 +447,7 @@ extern void handle_percpu_devid_irq(unsigned int irq,
struct irq_desc *desc);
extern void handle_bad_irq(unsigned int irq, struct irq_desc *desc);
extern void handle_nested_irq(unsigned int irq);
+extern int irq_chip_compose_msi_msg(struct irq_data *data, struct msi_msg
*msg);
#ifdef CONFIG_IRQ_DOMAIN_HIERARCHY
extern void irq_chip_ack_parent(struct irq_data *data);
extern int irq_chip_retrigger_hierarchy(struct irq_data *data);
diff --git a/kernel/irq/chip.c b/kernel/irq/chip.c
index 12f3e72449eb..8f362db17a8a 100644
--- a/kernel/irq/chip.c
+++ b/kernel/irq/chip.c
@@ -867,3 +867,20 @@ int irq_chip_retrigger_hierarchy(struct irq_data *data)
return -ENOSYS;
}
#endif
+
+int irq_chip_compose_msi_msg(struct irq_data *data, struct msi_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_msi_msg)
+ pos = data;
+ if (!pos)
+ return -ENOSYS;
+
+ pos-chip-irq_compose_msi_msg(pos, msg);
+
+ return 0;
+}
Adding message composing routine to struct irq_chip is OK to me, and it should
be because it is interrupt controllers' duty to compose messages (so that they
can parse the messages correctly without any pre-defined rules that endpoint
devices absolutely need not to know).
However a problem comes out when deciding which parameters should be passed to
this routine. A message can associate with multiple interrupts, which makes me
think composing messages for each interrupt is not that appropriate. And we
can take a look at the new routine irq_chip_compose_msi_msg(). It is called by
msi_domain_activate() which will be called by irq_domain_activate_irq() in
irq_startup() for each interrupt descriptor, result in composing a message for
each interrupt, right? (Unless requiring a judge on the parameter @data when
implementing the irq_compose_msi_msg() callback that only compose message for
the first entry of that message. But I really don't like that...)
Regards,
Abel
--
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 07/16] genirq: Introduce helper irq_domain_set_info() to reduce duplicated code
Hi Thomas, Jiang,
On 2014/11/12 21:43, Thomas Gleixner wrote:
From: Jiang Liu jiang@linux.intel.com
[...]
+void irq_domain_set_info(struct irq_domain *domain, unsigned int virq,
+ irq_hw_number_t hwirq, struct irq_chip *chip,
+ void *chip_data, irq_flow_handler_t handler,
+ void *handler_data, const char *handler_name)
+{
+ irq_domain_set_hwirq_and_chip(domain, virq, hwirq, chip, chip_data);
+ __irq_set_handler(virq, handler, 0, handler_name);
+ irq_set_handler_data(virq, handler_data);
+}
When stacked domain enabled, there will be a semantic shift to the linux
interrupt
identifiers. The @virq now delivers much more than before.
More specifically, now we need both @virq and @domain, rather than only @irq, to
determine which irq_data we want to configure. And once we configure @irq
without
providing the exact domain, it means we are configuring all the domains related
to
that @irq. So I think this routine just messed all things up.
Regards,
Abel
--
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 04/16] genirq: Introduce irq_chip.irq_compose_msi_msg() to support stacked irqchip
On 2014/11/18 18:02, Thomas Gleixner wrote:
On Tue, 18 Nov 2014, Yun Wu (Abel) wrote:
On 2014/11/12 21:42, Thomas Gleixner wrote:
+int irq_chip_compose_msi_msg(struct irq_data *data, struct msi_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_msi_msg)
+ pos = data;
+ if (!pos)
+ return -ENOSYS;
+
+ pos-chip-irq_compose_msi_msg(pos, msg);
+
+ return 0;
+}
Adding message composing routine to struct irq_chip is OK to me, and it
should
be because it is interrupt controllers' duty to compose messages (so that
they
can parse the messages correctly without any pre-defined rules that endpoint
devices absolutely need not to know).
However a problem comes out when deciding which parameters should be passed
to
this routine. A message can associate with multiple interrupts, which makes
me
think composing messages for each interrupt is not that appropriate. And we
can take a look at the new routine irq_chip_compose_msi_msg(). It is called
by
msi_domain_activate() which will be called by irq_domain_activate_irq() in
irq_startup() for each interrupt descriptor, result in composing a message
for
each interrupt, right? (Unless requiring a judge on the parameter @data when
implementing the irq_compose_msi_msg() callback that only compose message for
the first entry of that message. But I really don't like that...)
No, that's not correct. You are looking at some random stale version
of this. The current state of affairs is in
git://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git irq/irqdomain
See also https://lkml.org/lkml/2014/11/17/764
In activate we write the message, which is the right point to do so.
I checked the current state, it seems to be the same.
Yes, the decision of postponing the actual hardware programming to the point
where the interrupt actually gets used is right, but here above I was talking
another thing.
As I mentioned, a message can associate with multiple interrupts. Enabling
any of them will call irq_startup(). So if we don't want to compose or write
messages repeatedly, we'd better require performing some checks before
activating the interrupts.
Thanks,
Abel
--
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 07/16] genirq: Introduce helper irq_domain_set_info() to reduce duplicated code
On 2014/11/18 18:03, Thomas Gleixner wrote:
On Tue, 18 Nov 2014, Yun Wu (Abel) wrote:
Hi Thomas, Jiang,
On 2014/11/12 21:43, Thomas Gleixner wrote:
From: Jiang Liu jiang@linux.intel.com
[...]
+void irq_domain_set_info(struct irq_domain *domain, unsigned int virq,
+irq_hw_number_t hwirq, struct irq_chip *chip,
+void *chip_data, irq_flow_handler_t handler,
+void *handler_data, const char *handler_name)
+{
+ irq_domain_set_hwirq_and_chip(domain, virq, hwirq, chip, chip_data);
+ __irq_set_handler(virq, handler, 0, handler_name);
+ irq_set_handler_data(virq, handler_data);
+}
When stacked domain enabled, there will be a semantic shift to the linux
interrupt
identifiers. The @virq now delivers much more than before.
More specifically, now we need both @virq and @domain, rather than only
@irq, to
determine which irq_data we want to configure. And once we configure @irq
without
providing the exact domain, it means we are configuring all the domains
related to
that @irq. So I think this routine just messed all things up.
You can mess up anything by using an interface in the wrong way. Open
coding will not make that harder.
But what's the correct way to use this interface?
Thanks,
Abel
--
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 01/16] irqdomain: Introduce new interfaces to support hierarchy irqdomains
On 2014/11/18 17:54, Thomas Gleixner wrote: On Tue, 18 Nov 2014, Yun Wu (Abel) wrote: Hi Thomas, Jiang, On 2014/11/12 21:42, Thomas Gleixner wrote: From: Jiang Liu jiang@linux.intel.com Index: tip/kernel/irq/chip.c === --- tip.orig/kernel/irq/chip.c +++ tip/kernel/irq/chip.c @@ -15,6 +15,7 @@ #include linux/module.h #include linux/interrupt.h #include linux/kernel_stat.h +#include linux/irqdomain.h #include trace/events/irq.h @@ -178,6 +179,7 @@ int irq_startup(struct irq_desc *desc, b irq_state_clr_disabled(desc); desc-depth = 0; + irq_domain_activate_irq(desc-irq_data); I'm not sure why this is needed, please help me out.. :) Because it makes the whole error handling in stacked allocations way simpler. We allocate and reserve resources, but do not program the hardware up to the point where request_irq and therefor irq_startup() is invoked. So when in the allocation/reservation phase any of the stack level fails we just have to undo the allocations/reservations and not any hardware settings. That also solves the issue that depending on the stacking we might not be able to program the hardware during the allocation because all stack levels need to be allocated/reserved before we can figure out which hardware configuration we need for the various levels. So we decided to postpone the actual hardware programming to the point where the interrupt actually gets used. OK, I got it. Thanks, Abel -- 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 09/16] genirq: Add generic msi irq domain support
On 2014/11/12 21:43, Thomas Gleixner wrote:
From: Jiang Liu jiang@linux.intel.com
Implement the basic functions for MSI interrupt support with
hierarchical interrupt domains.
[ tglx: Extracted and combined from several patches ]
Signed-off-by: Jiang Liu jiang@linux.intel.com
Cc: Bjorn Helgaas bhelg...@google.com
Cc: Grant Likely grant.lik...@linaro.org
Cc: Marc Zyngier marc.zyng...@arm.com
Cc: Yingjoe Chen yingjoe.c...@mediatek.com
Cc: Yijing Wang wangyij...@huawei.com
Signed-off-by: Thomas Gleixner t...@linutronix.de
---
include/linux/msi.h | 35 +++
kernel/irq/Kconfig | 10
kernel/irq/Makefile |1
kernel/irq/msi.c| 119
4 files changed, 165 insertions(+)
Index: tip/include/linux/msi.h
===
--- tip.orig/include/linux/msi.h
+++ tip/include/linux/msi.h
@@ -77,4 +77,39 @@ struct msi_chip {
void (*teardown_irq)(struct msi_chip *chip, unsigned int irq);
};
+#ifdef CONFIG_GENERIC_MSI_IRQ_DOMAIN
+struct irq_domain;
+struct irq_chip;
+struct device_node;
+struct msi_domain_info;
+
+struct msi_domain_ops {
+ void(*calc_hwirq)(struct msi_domain_info *info, void *arg,
+ struct msi_desc *desc);
+ irq_hw_number_t (*get_hwirq)(struct msi_domain_info *info, void *arg);
+ int (*msi_init)(struct irq_domain *domain,
+ struct msi_domain_info *info,
+ unsigned int virq, irq_hw_number_t hwirq,
+ void *arg);
+ void(*msi_free)(struct irq_domain *domain,
+ struct msi_domain_info *info,
+ unsigned int virq);
+};
+
+struct msi_domain_info {
+ struct msi_domain_ops *ops;
+ struct irq_chip *chip;
+ void*data;
+};
+
+int msi_domain_set_affinity(struct irq_data *data, const struct cpumask
*mask,
+ bool force);
+
+struct irq_domain *msi_create_irq_domain(struct device_node *of_node,
+ struct msi_domain_info *info,
+ struct irq_domain *parent);
+struct msi_domain_info *msi_get_domain_info(struct irq_domain *domain);
+
+#endif /* CONFIG_GENERIC_MSI_IRQ_DOMAIN */
+
#endif /* LINUX_MSI_H */
Index: tip/kernel/irq/Kconfig
===
--- tip.orig/kernel/irq/Kconfig
+++ tip/kernel/irq/Kconfig
@@ -60,6 +60,16 @@ config IRQ_DOMAIN_HIERARCHY
bool
select IRQ_DOMAIN
+# Generic MSI interrupt support
+config GENERIC_MSI_IRQ
+ bool
+
+# Generic MSI hierarchical interrupt domain support
+config GENERIC_MSI_IRQ_DOMAIN
+ bool
+ select IRQ_DOMAIN_HIERARCHY
+ select GENERIC_MSI_IRQ
+
config HANDLE_DOMAIN_IRQ
bool
Index: tip/kernel/irq/Makefile
===
--- tip.orig/kernel/irq/Makefile
+++ tip/kernel/irq/Makefile
@@ -6,3 +6,4 @@ obj-$(CONFIG_IRQ_DOMAIN) += irqdomain.o
obj-$(CONFIG_PROC_FS) += proc.o
obj-$(CONFIG_GENERIC_PENDING_IRQ) += migration.o
obj-$(CONFIG_PM_SLEEP) += pm.o
+obj-$(CONFIG_GENERIC_MSI_IRQ) += msi.o
Index: tip/kernel/irq/msi.c
===
--- /dev/null
+++ tip/kernel/irq/msi.c
@@ -0,0 +1,119 @@
+/*
+ * linux/kernel/irq/msi.c
+ *
+ * Copyright (C) 2014 Intel Corp.
+ * Author: Jiang Liu jiang@linux.intel.com
+ *
+ * This file is licensed under GPLv2.
+ *
+ * This file contains common code to support Message Signalled Interrupt for
+ * PCI compatible and non PCI compatible devices.
+ */
+#include linux/irq.h
+#include linux/irqdomain.h
+#include linux/msi.h
+
+#ifdef CONFIG_GENERIC_MSI_IRQ_DOMAIN
+int msi_domain_set_affinity(struct irq_data *irq_data,
+ const struct cpumask *mask, bool force)
+{
+ struct irq_data *parent = irq_data-parent_data;
+ struct msi_msg msg;
+ int ret;
+
+ ret = parent-chip-irq_set_affinity(parent, mask, force);
+ if (ret = 0 ret != IRQ_SET_MASK_OK_DONE) {
+ BUG_ON(irq_chip_compose_msi_msg(irq_data, msg));
+ irq_chip_write_msi_msg(irq_data, msg);
+ }
+
+ return ret;
+}
+
+static void msi_domain_activate(struct irq_domain *domain,
+ struct irq_data *irq_data)
+{
+ struct msi_msg msg;
+
+ BUG_ON(irq_chip_compose_msi_msg(irq_data, msg));
+ irq_chip_write_msi_msg(irq_data, msg);
+}
+
+static void msi_domain_deactivate(struct irq_domain *domain,
+ struct irq_data *irq_data)
+{
+ struct msi_msg msg;
+
+ memset(msg, 0, sizeof(msg));
+
Re: [patch 04/16] genirq: Introduce irq_chip.irq_compose_msi_msg() to support stacked irqchip
On 2014/11/18 20:43, Jiang Liu wrote:
On 2014/11/18 19:47, Yun Wu (Abel) wrote:
On 2014/11/18 18:02, Thomas Gleixner wrote:
On Tue, 18 Nov 2014, Yun Wu (Abel) wrote:
On 2014/11/12 21:42, Thomas Gleixner wrote:
+int irq_chip_compose_msi_msg(struct irq_data *data, struct msi_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_msi_msg)
+ pos = data;
+ if (!pos)
+ return -ENOSYS;
+
+ pos-chip-irq_compose_msi_msg(pos, msg);
+
+ return 0;
+}
Adding message composing routine to struct irq_chip is OK to me, and it
should
be because it is interrupt controllers' duty to compose messages (so that
they
can parse the messages correctly without any pre-defined rules that
endpoint
devices absolutely need not to know).
However a problem comes out when deciding which parameters should be
passed to
this routine. A message can associate with multiple interrupts, which
makes me
think composing messages for each interrupt is not that appropriate. And we
can take a look at the new routine irq_chip_compose_msi_msg(). It is
called by
msi_domain_activate() which will be called by irq_domain_activate_irq() in
irq_startup() for each interrupt descriptor, result in composing a message
for
each interrupt, right? (Unless requiring a judge on the parameter @data
when
implementing the irq_compose_msi_msg() callback that only compose message
for
the first entry of that message. But I really don't like that...)
No, that's not correct. You are looking at some random stale version
of this. The current state of affairs is in
git://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git irq/irqdomain
See also https://lkml.org/lkml/2014/11/17/764
In activate we write the message, which is the right point to do so.
I checked the current state, it seems to be the same.
Yes, the decision of postponing the actual hardware programming to the point
where the interrupt actually gets used is right, but here above I was talking
another thing.
As I mentioned, a message can associate with multiple interrupts. Enabling
any of them will call irq_startup(). So if we don't want to compose or write
messages repeatedly, we'd better require performing some checks before
activating the interrupts.
Hi Yun,
Seems you are talking about the case of multiple MSI support.
Yes, we have special treatment for multiple MSI, which only writes PCI
MSI registers when starting up the first MSI interrupt.
void pci_msi_domain_write_msg(struct irq_data *irq_data, struct msi_msg
*msg)
{
struct msi_desc *desc = irq_data-msi_desc;
/*
* For MSI-X desc-irq is always equal to irq_data-irq. For
* MSI only the first interrupt of MULTI MSI passes the test.
*/
if (desc-irq == irq_data-irq)
__pci_write_msi_msg(desc, msg);
}
Yes, I picked the case of multiple MSI support.
The check should also be performed when composing messages. That's why
I don't like its parameters. The @data only indicates one interrupt,
while I prefer doing compose/write in the unit of message descriptor.
Thanks,
Abel
--
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 07/16] genirq: Introduce helper irq_domain_set_info() to reduce duplicated code
On 2014/11/18 20:38, Jiang Liu wrote:
On 2014/11/18 19:47, Yun Wu (Abel) wrote:
On 2014/11/18 18:03, Thomas Gleixner wrote:
On Tue, 18 Nov 2014, Yun Wu (Abel) wrote:
Hi Thomas, Jiang,
On 2014/11/12 21:43, Thomas Gleixner wrote:
From: Jiang Liu jiang@linux.intel.com
[...]
+void irq_domain_set_info(struct irq_domain *domain, unsigned int virq,
+ irq_hw_number_t hwirq, struct irq_chip *chip,
+ void *chip_data, irq_flow_handler_t handler,
+ void *handler_data, const char *handler_name)
+{
+ irq_domain_set_hwirq_and_chip(domain, virq, hwirq, chip, chip_data);
+ __irq_set_handler(virq, handler, 0, handler_name);
+ irq_set_handler_data(virq, handler_data);
+}
When stacked domain enabled, there will be a semantic shift to the linux
interrupt
identifiers. The @virq now delivers much more than before.
More specifically, now we need both @virq and @domain, rather than only
@irq, to
determine which irq_data we want to configure. And once we configure @irq
without
providing the exact domain, it means we are configuring all the domains
related to
that @irq. So I think this routine just messed all things up.
You can mess up anything by using an interface in the wrong way. Open
coding will not make that harder.
But what's the correct way to use this interface?
It's to be used by interrupt controller drivers to implement
hierarchy irqdomains.
Each time an interrupt domain calls this, the previous (@handler, @handler_name,
@handler_data) will be overrode. It's because the routines __irq_set_handler()
and irq_set_handler_data() only configure top level (without Marc's fix which is
not ideal). Is this what we really want to see?
Thanks,
Abel
--
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 08/16] genirq: Introduce callback irq_chip.irq_write_msi_msg
On 2014/11/18 18:19, Thomas Gleixner wrote:
On Tue, 18 Nov 2014, Yun Wu (Abel) wrote:
On 2014/11/12 21:43, Thomas Gleixner wrote:
struct irq_chip {
@@ -359,6 +360,7 @@ struct irq_chip {
void(*irq_release_resources)(struct irq_data *data);
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);
Hmm... It's really weird.
I don't think it's the interrupt controllers' responsibility to write
messages
for all the endpoint devices since the methods of configuring message
registers
may different between these devices. And theoretically, the endpoint devices
themselves should take the responsibility to configure their message
registers.
To say the least, the write_msg callback here still need to call some certain
interfaces provided by the corresponding device.
There would be lots of ARM new devices capable of sending message
based interrupts to interrupt controllers, does all the drivers of
the devices need to expose a write_msg callback to interrupt
controllers?
Well, writing the message _IS_ part of the interrupt controller.
So in order to enable non PCI based MSI we want to have generic
infrastructure with minimal per device/device class callbacks and of
course you need to provide that callback for your special device.
We already have non PCI based MSI controllers in x86 today and we need
to handle the whole stuff with tons of copied coded extra for each of
those. So consolidating it into common infrastructure allows us to get
rid of the pointless copied code and reduce the per device effort to
the relevant hardware specific callbacks. irq_write_msi_msg being one
of those.
At least, we have the same goal.
I will illustrate my thoughts by an example.
The current code is something like:
Device A
void A_write_msg() { ... }
Group B
(a group of devices behave same on writing messages, i.e. PCI)
===
void B_write_msg() { ... }
Controller
==
irq_chip.irq_write_msi_msg () {
if (A)
A_write_msg();
if (B)
B_write_msg();
}
It's horrible when new devices come out, since we need to modify the
controller part for each new device.
What I suggested is:
MSI Core
struct msi_ops { .write_msg, };
struct msi_desc { .msi_ops, };
write_msg() {
X = get_dev();
irq_chip.compose_msg(X);// IRQ chips' responsibility
X_msi_ops.write_msg(); // nothing to do with IRQ chips
}
Device A
void A_write_msg() { ... }
A_msi_ops.write_msg = A_write_msg;
Group B
===
void B_write_msg() { ... }
B_msi_ops.write_msg = B_write_msg;
Please correct me if I misunderstood anything.
Thanks,
Abel
--
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 04/16] genirq: Introduce irq_chip.irq_compose_msi_msg() to support stacked irqchip
On 2014/11/18 21:25, Jiang Liu wrote:
On 2014/11/18 21:16, Yun Wu (Abel) wrote:
On 2014/11/18 20:43, Jiang Liu wrote:
On 2014/11/18 19:47, Yun Wu (Abel) wrote:
On 2014/11/18 18:02, Thomas Gleixner wrote:
On Tue, 18 Nov 2014, Yun Wu (Abel) wrote:
On 2014/11/12 21:42, Thomas Gleixner wrote:
+int irq_chip_compose_msi_msg(struct irq_data *data, struct msi_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_msi_msg)
+ pos = data;
+ if (!pos)
+ return -ENOSYS;
+
+ pos-chip-irq_compose_msi_msg(pos, msg);
+
+ return 0;
+}
Adding message composing routine to struct irq_chip is OK to me, and it
should
be because it is interrupt controllers' duty to compose messages (so
that they
can parse the messages correctly without any pre-defined rules that
endpoint
devices absolutely need not to know).
However a problem comes out when deciding which parameters should be
passed to
this routine. A message can associate with multiple interrupts, which
makes me
think composing messages for each interrupt is not that appropriate. And
we
can take a look at the new routine irq_chip_compose_msi_msg(). It is
called by
msi_domain_activate() which will be called by irq_domain_activate_irq()
in
irq_startup() for each interrupt descriptor, result in composing a
message for
each interrupt, right? (Unless requiring a judge on the parameter @data
when
implementing the irq_compose_msi_msg() callback that only compose
message for
the first entry of that message. But I really don't like that...)
No, that's not correct. You are looking at some random stale version
of this. The current state of affairs is in
git://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git irq/irqdomain
See also https://lkml.org/lkml/2014/11/17/764
In activate we write the message, which is the right point to do so.
I checked the current state, it seems to be the same.
Yes, the decision of postponing the actual hardware programming to the
point
where the interrupt actually gets used is right, but here above I was
talking
another thing.
As I mentioned, a message can associate with multiple interrupts. Enabling
any of them will call irq_startup(). So if we don't want to compose or
write
messages repeatedly, we'd better require performing some checks before
activating the interrupts.
Hi Yun,
Seems you are talking about the case of multiple MSI support.
Yes, we have special treatment for multiple MSI, which only writes PCI
MSI registers when starting up the first MSI interrupt.
void pci_msi_domain_write_msg(struct irq_data *irq_data, struct msi_msg
*msg)
{
struct msi_desc *desc = irq_data-msi_desc;
/*
* For MSI-X desc-irq is always equal to irq_data-irq. For
* MSI only the first interrupt of MULTI MSI passes the test.
*/
if (desc-irq == irq_data-irq)
__pci_write_msi_msg(desc, msg);
}
Yes, I picked the case of multiple MSI support.
The check should also be performed when composing messages. That's why
I don't like its parameters. The @data only indicates one interrupt,
while I prefer doing compose/write in the unit of message descriptor.
Hi Yun,
The common abstraction is that every message interrupt could be
controlled independently, so have compose_msi_msg()/write_msi_msg() per
interrupt. MSI is abstracted as an special message signaled interrupt
with hardware limitation where multiple interrupts sharing the same
hardware registers. So we filter in pci_msi_domain_write_msg(). On the
other handle, the generic MSI framework caches msi_msg in msi_desc,
so we don't filter compose_msi_msg().
It's true that every message interrupt could be controlled independently,
I mean, by enable/disable/mask/unmask. But the message data address are
shared among the interrupts of that message.
Despite the detailed hardware implementation, MSI and MSI-X are the same
thing in software view, that is a message related with several consecutive
interrupts. And the core MSI infrastructure you want to build should not
be based on any hardware assumptions.
Thanks,
Abel
--
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 08/16] genirq: Introduce callback irq_chip.irq_write_msi_msg
On 2014/11/18 21:43, Jiang Liu wrote:
On 2014/11/18 21:33, Yun Wu (Abel) wrote:
On 2014/11/18 18:19, Thomas Gleixner wrote:
On Tue, 18 Nov 2014, Yun Wu (Abel) wrote:
On 2014/11/12 21:43, Thomas Gleixner wrote:
struct irq_chip {
@@ -359,6 +360,7 @@ struct irq_chip {
void(*irq_release_resources)(struct irq_data *data);
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);
Hmm... It's really weird.
I don't think it's the interrupt controllers' responsibility to write
messages
for all the endpoint devices since the methods of configuring message
registers
may different between these devices. And theoretically, the endpoint
devices
themselves should take the responsibility to configure their message
registers.
To say the least, the write_msg callback here still need to call some
certain
interfaces provided by the corresponding device.
There would be lots of ARM new devices capable of sending message
based interrupts to interrupt controllers, does all the drivers of
the devices need to expose a write_msg callback to interrupt
controllers?
Well, writing the message _IS_ part of the interrupt controller.
So in order to enable non PCI based MSI we want to have generic
infrastructure with minimal per device/device class callbacks and of
course you need to provide that callback for your special device.
We already have non PCI based MSI controllers in x86 today and we need
to handle the whole stuff with tons of copied coded extra for each of
those. So consolidating it into common infrastructure allows us to get
rid of the pointless copied code and reduce the per device effort to
the relevant hardware specific callbacks. irq_write_msi_msg being one
of those.
At least, we have the same goal.
I will illustrate my thoughts by an example.
The current code is something like:
Device A
void A_write_msg() { ... }
Group B
(a group of devices behave same on writing messages, i.e. PCI)
===
void B_write_msg() { ... }
Controller
==
irq_chip.irq_write_msi_msg () {
if (A)
A_write_msg();
if (B)
B_write_msg();
}
It's horrible when new devices come out, since we need to modify the
controller part for each new device.
What I suggested is:
MSI Core
struct msi_ops { .write_msg, };
struct msi_desc { .msi_ops, };
write_msg() {
X = get_dev();
irq_chip.compose_msg(X);// IRQ chips' responsibility
X_msi_ops.write_msg(); // nothing to do with IRQ chips
}
Device A
void A_write_msg() { ... }
A_msi_ops.write_msg = A_write_msg;
Group B
===
void B_write_msg() { ... }
B_msi_ops.write_msg = B_write_msg;
Please correct me if I misunderstood anything.
Hi Yun,
We provide an irq_chip for each type of interrupt controller
instead of devices. For the example mentioned above, if device A
and Group B has different interrupt controllers, we just need to
implement irq_chip_A and irq_chip_B and set irq_chip.irq_write_msi_msg()
to suitable callbacks.
The framework already achieves what you you want:)
What if device A and group B have the same interrupt controller?
Regards,
Abel
--
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 09/16] genirq: Add generic msi irq domain support
On 2014/11/18 20:49, Jiang Liu wrote:
On 2014/11/18 20:07, Yun Wu (Abel) wrote:
On 2014/11/12 21:43, Thomas Gleixner wrote:
From: Jiang Liu jiang@linux.intel.com
snip
+static int msi_domain_alloc(struct irq_domain *domain, unsigned int virq,
+ unsigned int nr_irqs, void *arg)
+{
+ struct msi_domain_info *info = domain-host_data;
+ struct msi_domain_ops *ops = info-ops;
+ irq_hw_number_t hwirq = ops-get_hwirq(info, arg);
+ int i, ret;
+
+ if (irq_find_mapping(domain, hwirq) 0)
+ return -EEXIST;
I think we need to check range here.
I fixed this by applying a patch showed in the bottom.
Hi Yun,
Yes, we have some assumptions here about the way the interface
will be used for. Will improve it in future patches.
Regards!
Gerry
OK. Just note that don't forget to check my second inline comment below. :)
+
+ ret = irq_domain_alloc_irqs_parent(domain, virq, nr_irqs, arg);
+ if (ret 0)
+ return ret;
+
+ for (i = 0; i nr_irqs; i++) {
+ ret = ops-msi_init(domain, info, virq + i, hwirq + i, arg);
+ if (ret 0) {
+ if (ops-msi_free) {
+ for (i--; i 0; i--)
while (i--) ?
Here.
Thanks,
Abel
+ ops-msi_free(domain, info, virq + i);
+ }
+ irq_domain_free_irqs_top(domain, virq, nr_irqs);
+ return ret;
+ }
+ }
+
+ return 0;
+}
+
+static void msi_domain_free(struct irq_domain *domain, unsigned int virq,
+ unsigned int nr_irqs)
+{
+ struct msi_domain_info *info = domain-host_data;
+ int i;
+
+ if (info-ops-msi_free) {
+ for (i = 0; i nr_irqs; i++)
+ info-ops-msi_free(domain, info, virq + i);
+ }
+ irq_domain_free_irqs_top(domain, virq, nr_irqs);
+}
+
+static struct irq_domain_ops msi_domain_ops = {
+ .alloc = msi_domain_alloc,
+ .free = msi_domain_free,
+ .activate = msi_domain_activate,
+ .deactivate = msi_domain_deactivate,
+};
+
+struct irq_domain *msi_create_irq_domain(struct device_node *of_node,
+struct msi_domain_info *info,
+struct irq_domain *parent)
+{
+ struct irq_domain *domain;
+
+ domain = irq_domain_add_tree(of_node, msi_domain_ops, info);
+ if (domain)
+ domain-parent = parent;
+
+ return domain;
+}
+
+struct msi_domain_info *msi_get_domain_info(struct irq_domain *domain)
+{
+ return (struct msi_domain_info *)domain-host_data;
+}
+
+#endif /* CONFIG_GENERIC_MSI_IRQ_DOMAIN */
diff --git a/kernel/irq/irqdomain.c b/kernel/irq/irqdomain.c
index cc18182..6d1be00 100644
--- a/kernel/irq/irqdomain.c
+++ b/kernel/irq/irqdomain.c
@@ -568,6 +568,34 @@ unsigned int irq_find_mapping(struct irq_domain *domain,
}
EXPORT_SYMBOL_GPL(irq_find_mapping);
+/**
+ * irq_find_mapping_many() - Find a range of linux irqs from a range of
hwirqs.
+ * @domain: domain owning these hardware interrupts
+ * @hwirq: start hardware irq number in @domain space
+ * @count: number of interrupts to find
+ *
+ * Returns the first linux irq number on success, or 0 when not found, or
error.
+ */
+int irq_find_mapping_many(struct irq_domain *domain, irq_hw_number_t hwirq,
+ int count)
+{
+unsigned int from;
+int i;
+
+for (i = from = 0; i count; i++) {
+if (!from) {
+from = irq_find_mapping(domain, hwirq + i);
+if (from i)
+return -1;
+} else if ((from + i) != irq_find_mapping(domain, hwirq + i)) {
+return -1;
+}
+}
+
+return from;
+}
+EXPORT_SYMBOL_GPL(irq_find_mapping_many);
+
#ifdef CONFIG_IRQ_DOMAIN_DEBUG
static int virq_debug_show(struct seq_file *m, void *private)
{
.
--
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 04/16] genirq: Introduce irq_chip.irq_compose_msi_msg() to support stacked irqchip
On 2014/11/18 21:55, Jiang Liu wrote:
On 2014/11/18 21:48, Yun Wu (Abel) wrote:
On 2014/11/18 21:25, Jiang Liu wrote:
On 2014/11/18 21:16, Yun Wu (Abel) wrote:
On 2014/11/18 20:43, Jiang Liu wrote:
On 2014/11/18 19:47, Yun Wu (Abel) wrote:
On 2014/11/18 18:02, Thomas Gleixner wrote:
On Tue, 18 Nov 2014, Yun Wu (Abel) wrote:
On 2014/11/12 21:42, Thomas Gleixner wrote:
+int irq_chip_compose_msi_msg(struct irq_data *data, struct msi_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_msi_msg)
+ pos = data;
+ if (!pos)
+ return -ENOSYS;
+
+ pos-chip-irq_compose_msi_msg(pos, msg);
+
+ return 0;
+}
Adding message composing routine to struct irq_chip is OK to me, and
it should
be because it is interrupt controllers' duty to compose messages (so
that they
can parse the messages correctly without any pre-defined rules that
endpoint
devices absolutely need not to know).
However a problem comes out when deciding which parameters should be
passed to
this routine. A message can associate with multiple interrupts, which
makes me
think composing messages for each interrupt is not that appropriate.
And we
can take a look at the new routine irq_chip_compose_msi_msg(). It is
called by
msi_domain_activate() which will be called by
irq_domain_activate_irq() in
irq_startup() for each interrupt descriptor, result in composing a
message for
each interrupt, right? (Unless requiring a judge on the parameter
@data when
implementing the irq_compose_msi_msg() callback that only compose
message for
the first entry of that message. But I really don't like that...)
No, that's not correct. You are looking at some random stale version
of this. The current state of affairs is in
git://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git
irq/irqdomain
See also https://lkml.org/lkml/2014/11/17/764
In activate we write the message, which is the right point to do so.
I checked the current state, it seems to be the same.
Yes, the decision of postponing the actual hardware programming to the
point
where the interrupt actually gets used is right, but here above I was
talking
another thing.
As I mentioned, a message can associate with multiple interrupts.
Enabling
any of them will call irq_startup(). So if we don't want to compose or
write
messages repeatedly, we'd better require performing some checks before
activating the interrupts.
Hi Yun,
Seems you are talking about the case of multiple MSI support.
Yes, we have special treatment for multiple MSI, which only writes PCI
MSI registers when starting up the first MSI interrupt.
void pci_msi_domain_write_msg(struct irq_data *irq_data, struct msi_msg
*msg)
{
struct msi_desc *desc = irq_data-msi_desc;
/*
* For MSI-X desc-irq is always equal to irq_data-irq. For
* MSI only the first interrupt of MULTI MSI passes the test.
*/
if (desc-irq == irq_data-irq)
__pci_write_msi_msg(desc, msg);
}
Yes, I picked the case of multiple MSI support.
The check should also be performed when composing messages. That's why
I don't like its parameters. The @data only indicates one interrupt,
while I prefer doing compose/write in the unit of message descriptor.
Hi Yun,
The common abstraction is that every message interrupt could be
controlled independently, so have compose_msi_msg()/write_msi_msg() per
interrupt. MSI is abstracted as an special message signaled interrupt
with hardware limitation where multiple interrupts sharing the same
hardware registers. So we filter in pci_msi_domain_write_msg(). On the
other handle, the generic MSI framework caches msi_msg in msi_desc,
so we don't filter compose_msi_msg().
It's true that every message interrupt could be controlled independently,
I mean, by enable/disable/mask/unmask. But the message data address are
shared among the interrupts of that message.
Despite the detailed hardware implementation, MSI and MSI-X are the same
thing in software view, that is a message related with several consecutive
interrupts. And the core MSI infrastructure you want to build should not
be based on any hardware assumptions.
That's the key point. We abstract MSI as using a message to control an
interrupt source instead of controlling several consecutive interrupts.
PCI MSI is just a special case which controls a group of consecutive
interrupts all together due to hardware limitation.
Oh, I see. We abstract it in different ways...
And sounds like you treat multiple MSI as a broken implementation?
Abel
--
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
Re: [patch 08/16] genirq: Introduce callback irq_chip.irq_write_msi_msg
On 2014/11/18 22:03, Jiang Liu wrote:
On 2014/11/18 21:52, Yun Wu (Abel) wrote:
On 2014/11/18 21:43, Jiang Liu wrote:
On 2014/11/18 21:33, Yun Wu (Abel) wrote:
On 2014/11/18 18:19, Thomas Gleixner wrote:
On Tue, 18 Nov 2014, Yun Wu (Abel) wrote:
On 2014/11/12 21:43, Thomas Gleixner wrote:
struct irq_chip {
@@ -359,6 +360,7 @@ struct irq_chip {
void(*irq_release_resources)(struct irq_data *data);
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);
Hmm... It's really weird.
I don't think it's the interrupt controllers' responsibility to write
messages
for all the endpoint devices since the methods of configuring message
registers
may different between these devices. And theoretically, the endpoint
devices
themselves should take the responsibility to configure their message
registers.
To say the least, the write_msg callback here still need to call some
certain
interfaces provided by the corresponding device.
There would be lots of ARM new devices capable of sending message
based interrupts to interrupt controllers, does all the drivers of
the devices need to expose a write_msg callback to interrupt
controllers?
Well, writing the message _IS_ part of the interrupt controller.
So in order to enable non PCI based MSI we want to have generic
infrastructure with minimal per device/device class callbacks and of
course you need to provide that callback for your special device.
We already have non PCI based MSI controllers in x86 today and we need
to handle the whole stuff with tons of copied coded extra for each of
those. So consolidating it into common infrastructure allows us to get
rid of the pointless copied code and reduce the per device effort to
the relevant hardware specific callbacks. irq_write_msi_msg being one
of those.
At least, we have the same goal.
I will illustrate my thoughts by an example.
The current code is something like:
Device A
void A_write_msg() { ... }
Group B
(a group of devices behave same on writing messages, i.e. PCI)
===
void B_write_msg() { ... }
Controller
==
irq_chip.irq_write_msi_msg () {
if (A)
A_write_msg();
if (B)
B_write_msg();
}
It's horrible when new devices come out, since we need to modify the
controller part for each new device.
What I suggested is:
MSI Core
struct msi_ops { .write_msg, };
struct msi_desc { .msi_ops, };
write_msg() {
X = get_dev();
irq_chip.compose_msg(X);// IRQ chips' responsibility
X_msi_ops.write_msg(); // nothing to do with IRQ chips
}
Device A
void A_write_msg() { ... }
A_msi_ops.write_msg = A_write_msg;
Group B
===
void B_write_msg() { ... }
B_msi_ops.write_msg = B_write_msg;
Please correct me if I misunderstood anything.
Hi Yun,
We provide an irq_chip for each type of interrupt controller
instead of devices. For the example mentioned above, if device A
and Group B has different interrupt controllers, we just need to
implement irq_chip_A and irq_chip_B and set irq_chip.irq_write_msi_msg()
to suitable callbacks.
The framework already achieves what you you want:)
What if device A and group B have the same interrupt controller?
Device doesn't care about interrupt controllers, it just cares about
interrupts used by itself. It's the interrupt source (controller)
enumerators' responsibility to discover interrupt source, associate
methods to control the interrupt source and assign irq numbers for
interrupt sources.
Yes, indeed.
There are two ways to associate irq numbers with device:
1) arch code/bus drivers statically assigns irq number for devices
when creating device objects, such as PCI legacy interrupt
(INTA-INTD), IOAPIC interrupts.
And OF interfaces.
2) device drivers ask interrupt source enumerators to dynamically
create irq numbers, such pci_enable_msix_range() and friends.
So device driver definitely doesn't need to known about irq_chip
methods to control interrupt sources.
The above you described is absolutely right, but not the things I want
to know. :)
Take GICv3 ITS for example, it deals with both PCI and non PCI message
interrupts. IIUC, several irq_chips need to be implemented in the ITS
driver (i.e. pci_msi_chip, A_msi_chip and B_msi_chip). What should we
do to the ITS driver if new MSI-capable devices come out?
Regards,
Abel
--
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/
