Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Subject: Re: [PATCH V14 10/15] irqchip/loongson-liointc: Add ACPI init support
To:     Marc Zyngier <maz@kernel.org>
Cc:     Thomas Gleixner <tglx@linutronix.de>, linux-kernel@vger.kernel.org,
        Hanjun Guo <guohanjun@huawei.com>,
        Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>,
        Jiaxun Yang <jiaxun.yang@flygoat.com>,
        Huacai Chen <chenhuacai@loongson.cn>
References: <1656837932-18257-1-git-send-email-lvjianmin@loongson.cn>
 <1656837932-18257-11-git-send-email-lvjianmin@loongson.cn>
 <874jztmctd.wl-maz@kernel.org>
From:   Jianmin Lv <lvjianmin@loongson.cn>
Message-ID: <9ab0b6ca-c515-a653-65c0-005af782da23@loongson.cn>
Date:   Fri, 8 Jul 2022 14:13:02 +0800
User-Agent: Mozilla/5.0 (X11; Linux loongarch64; rv:68.0) Gecko/20100101
 Thunderbird/68.7.0
MIME-Version: 1.0
In-Reply-To: <874jztmctd.wl-maz@kernel.org>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Language: en-US
Content-Transfer-Encoding: 8bit
Precedence: bulk


On 2022/7/7 下午6:34, Marc Zyngier wrote:
> On Sun, 03 Jul 2022 09:45:27 +0100,
> Jianmin Lv <lvjianmin@loongson.cn> wrote:
>>
>> From: Huacai Chen <chenhuacai@loongson.cn>
>>
>> LIOINTC stands for "Legacy I/O Interrupts" that described in Section
>> 11.1 of "Loongson 3A5000 Processor Reference Manual". For more
>> information please refer Documentation/loongarch/irq-chip-model.rst.
>>
>> Co-developed-by: Jianmin Lv <lvjianmin@loongson.cn>
>> Signed-off-by: Jianmin Lv <lvjianmin@loongson.cn>
>> Signed-off-by: Huacai Chen <chenhuacai@loongson.cn>
>> ---
>>   arch/loongarch/include/asm/irq.h       |   4 +-
>>   arch/loongarch/kernel/irq.c            |   1 -
>>   drivers/irqchip/irq-loongson-liointc.c | 227 ++++++++++++++++++++++-----------
>>   3 files changed, 154 insertions(+), 78 deletions(-)
>>
>> diff --git a/arch/loongarch/include/asm/irq.h b/arch/loongarch/include/asm/irq.h
>> index 8775dc6..b4c7956 100644
>> --- a/arch/loongarch/include/asm/irq.h
>> +++ b/arch/loongarch/include/asm/irq.h
>> @@ -97,7 +97,7 @@ static inline void eiointc_enable(void)
>>   
>>   struct irq_domain *loongarch_cpu_irq_init(void);
>>   
>> -struct irq_domain *liointc_acpi_init(struct irq_domain *parent,
>> +int liointc_acpi_init(struct irq_domain *parent,
>>   					struct acpi_madt_lio_pic *acpi_liointc);
>>   struct irq_domain *eiointc_acpi_init(struct irq_domain *parent,
>>   					struct acpi_madt_eio_pic *acpi_eiointc);
>> @@ -122,7 +122,7 @@ int pch_pic_acpi_init(struct irq_domain *parent,
>>   extern struct acpi_madt_bio_pic *acpi_pchpic[MAX_IO_PICS];
>>   
>>   extern struct irq_domain *cpu_domain;
>> -extern struct irq_domain *liointc_domain;
>> +extern struct fwnode_handle *liointc_handle;
>>   extern struct fwnode_handle *pch_lpc_handle;
>>   extern struct fwnode_handle *pch_pic_handle[MAX_IO_PICS];
>>   
>> diff --git a/arch/loongarch/kernel/irq.c b/arch/loongarch/kernel/irq.c
>> index ce21281..b04201c 100644
>> --- a/arch/loongarch/kernel/irq.c
>> +++ b/arch/loongarch/kernel/irq.c
>> @@ -26,7 +26,6 @@
>>   EXPORT_PER_CPU_SYMBOL(irq_stat);
>>   
>>   struct irq_domain *cpu_domain;
>> -struct irq_domain *liointc_domain;
>>   
>>   /*
>>    * 'what should we do if we get a hw irq event on an illegal vector'.
>> diff --git a/drivers/irqchip/irq-loongson-liointc.c b/drivers/irqchip/irq-loongson-liointc.c
>> index 8d05d8b..526ade4 100644
>> --- a/drivers/irqchip/irq-loongson-liointc.c
>> +++ b/drivers/irqchip/irq-loongson-liointc.c
>> @@ -23,7 +23,7 @@
>>   #endif
>>   
>>   #define LIOINTC_CHIP_IRQ	32
>> -#define LIOINTC_NUM_PARENT 4
>> +#define LIOINTC_NUM_PARENT	4
>>   #define LIOINTC_NUM_CORES	4
>>   
>>   #define LIOINTC_INTC_CHIP_START	0x20
>> @@ -58,6 +58,8 @@ struct liointc_priv {
>>   	bool				has_lpc_irq_errata;
>>   };
>>   
>> +struct fwnode_handle *liointc_handle;
>> +
>>   static void liointc_chained_handle_irq(struct irq_desc *desc)
>>   {
>>   	struct liointc_handler_data *handler = irq_desc_get_handler_data(desc);
>> @@ -153,97 +155,82 @@ static void liointc_resume(struct irq_chip_generic *gc)
>>   	irq_gc_unlock_irqrestore(gc, flags);
>>   }
>>   
>> -static const char * const parent_names[] = {"int0", "int1", "int2", "int3"};
>> -static const char * const core_reg_names[] = {"isr0", "isr1", "isr2", "isr3"};
>> +static int parent_irq[LIOINTC_NUM_PARENT];
>> +static u32 parent_int_map[LIOINTC_NUM_PARENT];
>> +static const char *const parent_names[] = {"int0", "int1", "int2", "int3"};
>> +static const char *const core_reg_names[] = {"isr0", "isr1", "isr2", "isr3"};
>>   
>> -static void __iomem *liointc_get_reg_byname(struct device_node *node,
>> -						const char *name)
>> +#ifdef CONFIG_ACPI
>> +static int liointc_domain_xlate(struct irq_domain *d, struct device_node *ctrlr,
>> +			     const u32 *intspec, unsigned int intsize,
>> +			     unsigned long *out_hwirq, unsigned int *out_type)
>>   {
>> -	int index = of_property_match_string(node, "reg-names", name);
>> -
>> -	if (index < 0)
>> -		return NULL;
>> -
>> -	return of_iomap(node, index);
>> +	if (WARN_ON(intsize < 1))
>> +		return -EINVAL;
>> +	*out_hwirq = intspec[0] - GSI_MIN_CPU_IRQ;
>> +	*out_type = IRQ_TYPE_NONE;
>> +	return 0;
>>   }
>>   
>> -static int __init liointc_of_init(struct device_node *node,
>> -				  struct device_node *parent)
>> +const struct irq_domain_ops acpi_irq_gc_ops = {
> 
> This should be static.
> 

Ok, thanks, I'll change it.


>> +	.map	= irq_map_generic_chip,
>> +	.unmap  = irq_unmap_generic_chip,
>> +	.xlate	= liointc_domain_xlate,
>> +};
>> +#endif
>> +
>> +static int liointc_init(phys_addr_t addr, unsigned long size, int revision,
>> +		struct fwnode_handle *domain_handle, struct device_node *node)
>>   {
>> +	int i, err;
>> +	void __iomem *base;
>> +	struct irq_chip_type *ct;
>>   	struct irq_chip_generic *gc;
>>   	struct irq_domain *domain;
>> -	struct irq_chip_type *ct;
>>   	struct liointc_priv *priv;
>> -	void __iomem *base;
>> -	u32 of_parent_int_map[LIOINTC_NUM_PARENT];
>> -	int parent_irq[LIOINTC_NUM_PARENT];
>> -	bool have_parent = FALSE;
>> -	int sz, i, err = 0;
>>   
>>   	priv = kzalloc(sizeof(*priv), GFP_KERNEL);
>>   	if (!priv)
>>   		return -ENOMEM;
>>   
>> -	if (of_device_is_compatible(node, "loongson,liointc-2.0")) {
>> -		base = liointc_get_reg_byname(node, "main");
>> -		if (!base) {
>> -			err = -ENODEV;
>> -			goto out_free_priv;
>> -		}
>> +	base = ioremap(addr, size);
>> +	if (!base)
>> +		goto out_free_priv;
>>   
>> -		for (i = 0; i < LIOINTC_NUM_CORES; i++)
>> -			priv->core_isr[i] = liointc_get_reg_byname(node, core_reg_names[i]);
>> -		if (!priv->core_isr[0]) {
>> -			err = -ENODEV;
>> -			goto out_iounmap_base;
>> -		}
>> -	} else {
>> -		base = of_iomap(node, 0);
>> -		if (!base) {
>> -			err = -ENODEV;
>> -			goto out_free_priv;
>> -		}
>> +	for (i = 0; i < LIOINTC_NUM_CORES; i++)
>> +		priv->core_isr[i] = base + LIOINTC_REG_INTC_STATUS;
>>   
>> -		for (i = 0; i < LIOINTC_NUM_CORES; i++)
>> -			priv->core_isr[i] = base + LIOINTC_REG_INTC_STATUS;
>> -	}
>> +	for (i = 0; i < LIOINTC_NUM_PARENT; i++)
>> +		priv->handler[i].parent_int_map = parent_int_map[i];
>>   
>> -	for (i = 0; i < LIOINTC_NUM_PARENT; i++) {
>> -		parent_irq[i] = of_irq_get_byname(node, parent_names[i]);
>> -		if (parent_irq[i] > 0)
>> -			have_parent = TRUE;
>> -	}
>> -	if (!have_parent) {
>> -		err = -ENODEV;
>> -		goto out_iounmap_isr;
>> -	}
>> +	if (revision > 1) {
>> +		for (i = 0; i < LIOINTC_NUM_CORES; i++) {
>> +			int index = of_property_match_string(node,
>> +					"reg-names", core_reg_names[i]);
>>   
>> -	sz = of_property_read_variable_u32_array(node,
>> -						"loongson,parent_int_map",
>> -						&of_parent_int_map[0],
>> -						LIOINTC_NUM_PARENT,
>> -						LIOINTC_NUM_PARENT);
>> -	if (sz < 4) {
>> -		pr_err("loongson-liointc: No parent_int_map\n");
>> -		err = -ENODEV;
>> -		goto out_iounmap_isr;
>> -	}
>> +			if (index < 0)
>> +				return -EINVAL;
>>   
>> -	for (i = 0; i < LIOINTC_NUM_PARENT; i++)
>> -		priv->handler[i].parent_int_map = of_parent_int_map[i];
>> +			priv->core_isr[i] = of_iomap(node, index);
>> +		}
>> +	}
>>   
>>   	/* Setup IRQ domain */
>> -	domain = irq_domain_add_linear(node, 32,
>> +#ifdef CONFIG_ACPI
>> +	domain = irq_domain_create_linear(domain_handle, LIOINTC_CHIP_IRQ,
>> +					&acpi_irq_gc_ops, priv);
>> +#else
>> +	domain = irq_domain_create_linear(domain_handle, LIOINTC_CHIP_IRQ,
>>   					&irq_generic_chip_ops, priv);
>> +#endif
> 
> This feels wrong. The ACPI vs OF decision should be a runtime
> one. Maybe you don't have that issue yet because MIPS is only OF and
> LoongArch only ACPI, but it remains that this should be done
> dynamically.
> 
> Please try to rewrite it as:
> 
> 	if (!acpi_disabled)
> 		[... ACPI stuff ...]
> 	else
> 		[... OF stuff ...]
> 
> This should apply to all other cases where you have similar distinctions.
> 

Ok, thanks, I'll change all the patch series according to your 
suggestion here.


> Thanks,
> 
> 	M.
>