[PATCH v1] irqchip: loongson-liointc: add hierarchy irq support

[PATCH v1] irqchip: loongson-liointc: add hierarchy irq support

[Yinbo Zhu]

When the irq of hierarchical interrupt chip was routed to liointc
that asked liointc driver to support hierarchy irq and this patch
was to add such support.

In addition, this patch only consider dts, and acpi hierarchy irq
support will be added later as required.

Signed-off-by: Yinbo Zhu <zhuyinbo@loongson.cn>
---
 drivers/irqchip/irq-loongson-liointc.c | 31 ++++++++++++++++++++++++++
 1 file changed, 31 insertions(+)

diff --git a/drivers/irqchip/irq-loongson-liointc.c b/drivers/irqchip/irq-loongson-liointc.c
index 0da8716f8f24..58e43a2cd02e 100644
--- a/drivers/irqchip/irq-loongson-liointc.c
+++ b/drivers/irqchip/irq-loongson-liointc.c
@@ -177,6 +177,32 @@ static const struct irq_domain_ops acpi_irq_gc_ops = {
 	.xlate	= liointc_domain_xlate,
 };
 
+#ifdef	CONFIG_IRQ_DOMAIN_HIERARCHY
+static int liointc_domain_alloc(struct irq_domain *domain, unsigned int virq,
+				unsigned int nr_irqs, void *arg)
+{
+	int i, ret;
+	irq_hw_number_t hwirq;
+	unsigned int type = IRQ_TYPE_NONE;
+	struct irq_fwspec *fwspec = arg;
+
+	ret = irq_domain_translate_twocell(domain, fwspec, &hwirq, &type);
+	if (ret)
+		return ret;
+
+	for (i = 0; i < nr_irqs; i++)
+		irq_map_generic_chip(domain, virq + i, hwirq + i);
+
+	return 0;
+}
+
+static const struct irq_domain_ops of_irq_gc_ops = {
+	.translate	= irq_domain_translate_twocell,
+	.alloc		= liointc_domain_alloc,
+	.free		= irq_domain_free_irqs_top,
+};
+#endif
+
 static int liointc_init(phys_addr_t addr, unsigned long size, int revision,
 		struct fwnode_handle *domain_handle, struct device_node *node)
 {
@@ -218,8 +244,13 @@ static int liointc_init(phys_addr_t addr, unsigned long size, int revision,
 		domain = irq_domain_create_linear(domain_handle, LIOINTC_CHIP_IRQ,
 					&acpi_irq_gc_ops, priv);
 	else
+#ifdef	CONFIG_IRQ_DOMAIN_HIERARCHY
+		domain = irq_domain_create_linear(domain_handle, LIOINTC_CHIP_IRQ,
+					&of_irq_gc_ops, priv);
+#else
 		domain = irq_domain_create_linear(domain_handle, LIOINTC_CHIP_IRQ,
 					&irq_generic_chip_ops, priv);
+#endif
 	if (!domain) {
 		pr_err("loongson-liointc: cannot add IRQ domain\n");
 		goto out_iounmap;
-- 
2.31.1

Re: [PATCH v1] irqchip: loongson-liointc: add hierarchy irq support

[Marc Zyngier]

On Wed, 07 Dec 2022 01:45:55 +0000,
Yinbo Zhu <zhuyinbo@loongson.cn> wrote:
> 
> When the irq of hierarchical interrupt chip was routed to liointc
> that asked liointc driver to support hierarchy irq and this patch
> was to add such support.
> 
> In addition, this patch only consider dts, and acpi hierarchy irq
> support will be added later as required.
> 
> Signed-off-by: Yinbo Zhu <zhuyinbo@loongson.cn>
> ---
>  drivers/irqchip/irq-loongson-liointc.c | 31 ++++++++++++++++++++++++++
>  1 file changed, 31 insertions(+)
> 
> diff --git a/drivers/irqchip/irq-loongson-liointc.c b/drivers/irqchip/irq-loongson-liointc.c
> index 0da8716f8f24..58e43a2cd02e 100644
> --- a/drivers/irqchip/irq-loongson-liointc.c
> +++ b/drivers/irqchip/irq-loongson-liointc.c
> @@ -177,6 +177,32 @@ static const struct irq_domain_ops acpi_irq_gc_ops = {
>  	.xlate	= liointc_domain_xlate,
>  };
>  
> +#ifdef	CONFIG_IRQ_DOMAIN_HIERARCHY
> +static int liointc_domain_alloc(struct irq_domain *domain, unsigned int virq,
> +				unsigned int nr_irqs, void *arg)
> +{
> +	int i, ret;
> +	irq_hw_number_t hwirq;
> +	unsigned int type = IRQ_TYPE_NONE;
> +	struct irq_fwspec *fwspec = arg;
> +
> +	ret = irq_domain_translate_twocell(domain, fwspec, &hwirq, &type);
> +	if (ret)
> +		return ret;
> +
> +	for (i = 0; i < nr_irqs; i++)
> +		irq_map_generic_chip(domain, virq + i, hwirq + i);
> +
> +	return 0;
> +}
> +
> +static const struct irq_domain_ops of_irq_gc_ops = {
> +	.translate	= irq_domain_translate_twocell,
> +	.alloc		= liointc_domain_alloc,
> +	.free		= irq_domain_free_irqs_top,
> +};
> +#endif
> +
>  static int liointc_init(phys_addr_t addr, unsigned long size, int revision,
>  		struct fwnode_handle *domain_handle, struct device_node *node)
>  {
> @@ -218,8 +244,13 @@ static int liointc_init(phys_addr_t addr, unsigned long size, int revision,
>  		domain = irq_domain_create_linear(domain_handle, LIOINTC_CHIP_IRQ,
>  					&acpi_irq_gc_ops, priv);
>  	else
> +#ifdef	CONFIG_IRQ_DOMAIN_HIERARCHY
> +		domain = irq_domain_create_linear(domain_handle, LIOINTC_CHIP_IRQ,
> +					&of_irq_gc_ops, priv);
> +#else
>  		domain = irq_domain_create_linear(domain_handle, LIOINTC_CHIP_IRQ,
>  					&irq_generic_chip_ops, priv);
> +#endif

Two things:

- Why do we need three calls to create the same domains depending on
  what firmware is used and kernel configuration?

- who is going to decide whether to select the
  CONFIG_IRQ_DOMAIN_HIERARCHY option?

I'd really like to see a statement from the Loongson folks about what
this whole DT stuff is all about. AFAICT, the core ACPICA stuff isn't
even fully merged (i.e. we still rely on arch-specific hacks).

Can you *please* finish what you've started before adding another
layer of quality stuff on top?

	M.

-- 
Without deviation from the norm, progress is not possible.

Re: [PATCH v1] irqchip: loongson-liointc: add hierarchy irq support

[Yinbo Zhu]

在 2022/12/7 16:08, Marc Zyngier 写道:
> On Wed, 07 Dec 2022 01:45:55 +0000,
> Yinbo Zhu <zhuyinbo@loongson.cn> wrote:
>> When the irq of hierarchical interrupt chip was routed to liointc
>> that asked liointc driver to support hierarchy irq and this patch
>> was to add such support.
>>
>> In addition, this patch only consider dts, and acpi hierarchy irq
>> support will be added later as required.
>>
>> Signed-off-by: Yinbo Zhu <zhuyinbo@loongson.cn>
>> ---
>>   drivers/irqchip/irq-loongson-liointc.c | 31 ++++++++++++++++++++++++++
>>   1 file changed, 31 insertions(+)
>>
>> diff --git a/drivers/irqchip/irq-loongson-liointc.c b/drivers/irqchip/irq-loongson-liointc.c
>> index 0da8716f8f24..58e43a2cd02e 100644
>> --- a/drivers/irqchip/irq-loongson-liointc.c
>> +++ b/drivers/irqchip/irq-loongson-liointc.c
>> @@ -177,6 +177,32 @@ static const struct irq_domain_ops acpi_irq_gc_ops = {
>>   	.xlate	= liointc_domain_xlate,
>>   };
>>   
>> +#ifdef	CONFIG_IRQ_DOMAIN_HIERARCHY
>> +static int liointc_domain_alloc(struct irq_domain *domain, unsigned int virq,
>> +				unsigned int nr_irqs, void *arg)
>> +{
>> +	int i, ret;
>> +	irq_hw_number_t hwirq;
>> +	unsigned int type = IRQ_TYPE_NONE;
>> +	struct irq_fwspec *fwspec = arg;
>> +
>> +	ret = irq_domain_translate_twocell(domain, fwspec, &hwirq, &type);
>> +	if (ret)
>> +		return ret;
>> +
>> +	for (i = 0; i < nr_irqs; i++)
>> +		irq_map_generic_chip(domain, virq + i, hwirq + i);
>> +
>> +	return 0;
>> +}
>> +
>> +static const struct irq_domain_ops of_irq_gc_ops = {
>> +	.translate	= irq_domain_translate_twocell,
>> +	.alloc		= liointc_domain_alloc,
>> +	.free		= irq_domain_free_irqs_top,
>> +};
>> +#endif
>> +
>>   static int liointc_init(phys_addr_t addr, unsigned long size, int revision,
>>   		struct fwnode_handle *domain_handle, struct device_node *node)
>>   {
>> @@ -218,8 +244,13 @@ static int liointc_init(phys_addr_t addr, unsigned long size, int revision,
>>   		domain = irq_domain_create_linear(domain_handle, LIOINTC_CHIP_IRQ,
>>   					&acpi_irq_gc_ops, priv);
>>   	else
>> +#ifdef	CONFIG_IRQ_DOMAIN_HIERARCHY
>> +		domain = irq_domain_create_linear(domain_handle, LIOINTC_CHIP_IRQ,
>> +					&of_irq_gc_ops, priv);
>> +#else
>>   		domain = irq_domain_create_linear(domain_handle, LIOINTC_CHIP_IRQ,
>>   					&irq_generic_chip_ops, priv);
>> +#endif
> Two things:
>
> - Why do we need three calls to create the same domains depending on
>    what firmware is used and kernel configuration?
yes, It depend on firmeware and kernel configuration.
>
> - who is going to decide whether to select the
>    CONFIG_IRQ_DOMAIN_HIERARCHY option?
The latest gpio driver will select  CONFIG_IRQ_DOMAIN_HIERARCHY
>
> I'd really like to see a statement from the Loongson folks about what
> this whole DT stuff is all about. AFAICT, the core ACPICA stuff isn't
> even fully merged (i.e. we still rely on arch-specific hacks).

The support of dts is mainly for Loongson embedded chips, such as 
LoongArch Loongson-2

series SoC.  and it use dts to descripte device and don't support acpi.

>
> Can you *please* finish what you've started before adding another
> layer of quality stuff on top?
>
> 	M.
>

Re: [PATCH v1] irqchip: loongson-liointc: add hierarchy irq support

[Marc Zyngier]

On Wed, 07 Dec 2022 10:50:56 +0000,
Yinbo Zhu <zhuyinbo@loongson.cn> wrote:
> 
> 
> 在 2022/12/7 16:08, Marc Zyngier 写道:
> > On Wed, 07 Dec 2022 01:45:55 +0000,
> > Yinbo Zhu <zhuyinbo@loongson.cn> wrote:
> >> When the irq of hierarchical interrupt chip was routed to liointc
> >> that asked liointc driver to support hierarchy irq and this patch
> >> was to add such support.
> >> 
> >> In addition, this patch only consider dts, and acpi hierarchy irq
> >> support will be added later as required.
> >> 
> >> Signed-off-by: Yinbo Zhu <zhuyinbo@loongson.cn>
> >> ---
> >>   drivers/irqchip/irq-loongson-liointc.c | 31 ++++++++++++++++++++++++++
> >>   1 file changed, 31 insertions(+)
> >> 
> >> diff --git a/drivers/irqchip/irq-loongson-liointc.c b/drivers/irqchip/irq-loongson-liointc.c
> >> index 0da8716f8f24..58e43a2cd02e 100644
> >> --- a/drivers/irqchip/irq-loongson-liointc.c
> >> +++ b/drivers/irqchip/irq-loongson-liointc.c
> >> @@ -177,6 +177,32 @@ static const struct irq_domain_ops acpi_irq_gc_ops = {
> >>   	.xlate	= liointc_domain_xlate,
> >>   };
> >>   +#ifdef	CONFIG_IRQ_DOMAIN_HIERARCHY
> >> +static int liointc_domain_alloc(struct irq_domain *domain, unsigned int virq,
> >> +				unsigned int nr_irqs, void *arg)
> >> +{
> >> +	int i, ret;
> >> +	irq_hw_number_t hwirq;
> >> +	unsigned int type = IRQ_TYPE_NONE;
> >> +	struct irq_fwspec *fwspec = arg;
> >> +
> >> +	ret = irq_domain_translate_twocell(domain, fwspec, &hwirq, &type);
> >> +	if (ret)
> >> +		return ret;
> >> +
> >> +	for (i = 0; i < nr_irqs; i++)
> >> +		irq_map_generic_chip(domain, virq + i, hwirq + i);
> >> +
> >> +	return 0;
> >> +}
> >> +
> >> +static const struct irq_domain_ops of_irq_gc_ops = {
> >> +	.translate	= irq_domain_translate_twocell,
> >> +	.alloc		= liointc_domain_alloc,
> >> +	.free		= irq_domain_free_irqs_top,
> >> +};
> >> +#endif
> >> +
> >>   static int liointc_init(phys_addr_t addr, unsigned long size, int revision,
> >>   		struct fwnode_handle *domain_handle, struct device_node *node)
> >>   {
> >> @@ -218,8 +244,13 @@ static int liointc_init(phys_addr_t addr, unsigned long size, int revision,
> >>   		domain = irq_domain_create_linear(domain_handle, LIOINTC_CHIP_IRQ,
> >>   					&acpi_irq_gc_ops, priv);
> >>   	else
> >> +#ifdef	CONFIG_IRQ_DOMAIN_HIERARCHY
> >> +		domain = irq_domain_create_linear(domain_handle, LIOINTC_CHIP_IRQ,
> >> +					&of_irq_gc_ops, priv);
> >> +#else
> >>   		domain = irq_domain_create_linear(domain_handle, LIOINTC_CHIP_IRQ,
> >>   					&irq_generic_chip_ops, priv);
> >> +#endif
> > Two things:
> > 
> > - Why do we need three calls to create the same domains depending on
> >    what firmware is used and kernel configuration?
> yes, It depend on firmeware and kernel configuration.

Read again:

why do we need 3 different calls to irq_domain_create_linear when you
can *indirect* them with a pointer to the correct structure?

> > 
> > - who is going to decide whether to select the
> >    CONFIG_IRQ_DOMAIN_HIERARCHY option?
> The latest gpio driver will select  CONFIG_IRQ_DOMAIN_HIERARCHY

Then why do we need two different behaviours? The same kernel should
run everywhere.

> > 
> > I'd really like to see a statement from the Loongson folks about what
> > this whole DT stuff is all about. AFAICT, the core ACPICA stuff isn't
> > even fully merged (i.e. we still rely on arch-specific hacks).
> 
> The support of dts is mainly for Loongson embedded chips, such as
> LoongArch Loongson-2 series SoC.  and it use dts to descripte device
> and don't support acpi.

That doesn't answer my question. Please have a *consistent* approach
to your interrupt handling, and work with your ACPI colleagues.

	M.

-- 
Without deviation from the norm, progress is not possible.

Re: [PATCH v1] irqchip: loongson-liointc: add hierarchy irq support

[Yinbo Zhu]

在 2022/12/7 19:07, Marc Zyngier 写道:
> On Wed, 07 Dec 2022 10:50:56 +0000,
> Yinbo Zhu <zhuyinbo@loongson.cn> wrote:
>>
>> 在 2022/12/7 16:08, Marc Zyngier 写道:
>>> On Wed, 07 Dec 2022 01:45:55 +0000,
>>> Yinbo Zhu <zhuyinbo@loongson.cn> wrote:
>>>> When the irq of hierarchical interrupt chip was routed to liointc
>>>> that asked liointc driver to support hierarchy irq and this patch
>>>> was to add such support.
>>>>
>>>> In addition, this patch only consider dts, and acpi hierarchy irq
>>>> support will be added later as required.
>>>>
>>>> Signed-off-by: Yinbo Zhu <zhuyinbo@loongson.cn>
>>>> ---
>>>>    drivers/irqchip/irq-loongson-liointc.c | 31 ++++++++++++++++++++++++++
>>>>    1 file changed, 31 insertions(+)
>>>>
>>>> diff --git a/drivers/irqchip/irq-loongson-liointc.c b/drivers/irqchip/irq-loongson-liointc.c
>>>> index 0da8716f8f24..58e43a2cd02e 100644
>>>> --- a/drivers/irqchip/irq-loongson-liointc.c
>>>> +++ b/drivers/irqchip/irq-loongson-liointc.c
>>>> @@ -177,6 +177,32 @@ static const struct irq_domain_ops acpi_irq_gc_ops = {
>>>>    	.xlate	= liointc_domain_xlate,
>>>>    };
>>>>    +#ifdef	CONFIG_IRQ_DOMAIN_HIERARCHY
>>>> +static int liointc_domain_alloc(struct irq_domain *domain, unsigned int virq,
>>>> +				unsigned int nr_irqs, void *arg)
>>>> +{
>>>> +	int i, ret;
>>>> +	irq_hw_number_t hwirq;
>>>> +	unsigned int type = IRQ_TYPE_NONE;
>>>> +	struct irq_fwspec *fwspec = arg;
>>>> +
>>>> +	ret = irq_domain_translate_twocell(domain, fwspec, &hwirq, &type);
>>>> +	if (ret)
>>>> +		return ret;
>>>> +
>>>> +	for (i = 0; i < nr_irqs; i++)
>>>> +		irq_map_generic_chip(domain, virq + i, hwirq + i);
>>>> +
>>>> +	return 0;
>>>> +}
>>>> +
>>>> +static const struct irq_domain_ops of_irq_gc_ops = {
>>>> +	.translate	= irq_domain_translate_twocell,
>>>> +	.alloc		= liointc_domain_alloc,
>>>> +	.free		= irq_domain_free_irqs_top,
>>>> +};
>>>> +#endif
>>>> +
>>>>    static int liointc_init(phys_addr_t addr, unsigned long size, int revision,
>>>>    		struct fwnode_handle *domain_handle, struct device_node *node)
>>>>    {
>>>> @@ -218,8 +244,13 @@ static int liointc_init(phys_addr_t addr, unsigned long size, int revision,
>>>>    		domain = irq_domain_create_linear(domain_handle, LIOINTC_CHIP_IRQ,
>>>>    					&acpi_irq_gc_ops, priv);
>>>>    	else
>>>> +#ifdef	CONFIG_IRQ_DOMAIN_HIERARCHY
>>>> +		domain = irq_domain_create_linear(domain_handle, LIOINTC_CHIP_IRQ,
>>>> +					&of_irq_gc_ops, priv);
>>>> +#else
>>>>    		domain = irq_domain_create_linear(domain_handle, LIOINTC_CHIP_IRQ,
>>>>    					&irq_generic_chip_ops, priv);
>>>> +#endif
>>> Two things:
>>>
>>> - Why do we need three calls to create the same domains depending on
>>>     what firmware is used and kernel configuration?
>> yes, It depend on firmeware and kernel configuration.
> Read again:
>
> why do we need 3 different calls to irq_domain_create_linear when you
> can *indirect* them with a pointer to the correct structure?
It was not considered comprehensively before, one call is enough.
>
>>> - who is going to decide whether to select the
>>>     CONFIG_IRQ_DOMAIN_HIERARCHY option?
>> The latest gpio driver will select  CONFIG_IRQ_DOMAIN_HIERARCHY
> Then why do we need two different behaviours? The same kernel should
> run everywhere.

in fact, A behaviours can handle it, and I will add proper change

in v2.

>
>>> I'd really like to see a statement from the Loongson folks about what
>>> this whole DT stuff is all about. AFAICT, the core ACPICA stuff isn't
>>> even fully merged (i.e. we still rely on arch-specific hacks).
>> The support of dts is mainly for Loongson embedded chips, such as
>> LoongArch Loongson-2 series SoC.  and it use dts to descripte device
>> and don't support acpi.
> That doesn't answer my question. Please have a *consistent* approach
> to your interrupt handling, and work with your ACPI colleagues.

I have a talk with ACPI colleagues that the dts and acpi can keep consistent

approach and I will add it in v2.

>
> 	M.
>