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 V13 05/13] irqchip: Add Loongson PCH LPC controller
 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: <1656329997-20524-1-git-send-email-lvjianmin@loongson.cn>
 <1656329997-20524-6-git-send-email-lvjianmin@loongson.cn>
 <87wncz20st.wl-maz@kernel.org>
From:   Jianmin Lv <lvjianmin@loongson.cn>
Message-ID: <4366e5a0-0ba6-52fe-3f68-ce45c3ad1ea7@loongson.cn>
Date:   Thu, 30 Jun 2022 12:40:26 +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: <87wncz20st.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/6/29 下午6:58, Marc Zyngier wrote:
> On Mon, 27 Jun 2022 12:39:49 +0100,
> Jianmin Lv <lvjianmin@loongson.cn> wrote:
>>
>> From: Huacai Chen <chenhuacai@loongson.cn>
>>
>> PCH-LPC stands for "LPC Interrupts" that described in Section 24.3 of
>> "Loongson 7A1000 Bridge User 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/Kconfig                |   8 ++
>>   drivers/irqchip/Makefile               |   1 +
>>   drivers/irqchip/irq-loongson-pch-lpc.c | 203 +++++++++++++++++++++++++++++++++
>>   5 files changed, 214 insertions(+), 3 deletions(-)
>>   create mode 100644 drivers/irqchip/irq-loongson-pch-lpc.c
>>
>> diff --git a/arch/loongarch/include/asm/irq.h b/arch/loongarch/include/asm/irq.h
>> index ace3ea6..48c0ce4 100644
>> --- a/arch/loongarch/include/asm/irq.h
>> +++ b/arch/loongarch/include/asm/irq.h
>> @@ -104,7 +104,7 @@ struct irq_domain *eiointc_acpi_init(struct irq_domain *parent,
>>   
>>   struct irq_domain *htvec_acpi_init(struct irq_domain *parent,
>>   					struct acpi_madt_ht_pic *acpi_htvec);
>> -struct irq_domain *pch_lpc_acpi_init(struct irq_domain *parent,
>> +int pch_lpc_acpi_init(struct irq_domain *parent,
>>   					struct acpi_madt_lpc_pic *acpi_pchlpc);
>>   struct irq_domain *pch_msi_acpi_init(struct irq_domain *parent,
>>   					struct acpi_madt_msi_pic *acpi_pchmsi);
>> @@ -121,7 +121,7 @@ struct irq_domain *pch_pic_acpi_init(struct irq_domain *parent,
>>   
>>   extern struct irq_domain *cpu_domain;
>>   extern struct irq_domain *liointc_domain;
>> -extern struct irq_domain *pch_lpc_domain;
>> +extern struct fwnode_handle *pch_lpc_handle;
>>   extern struct irq_domain *pch_msi_domain[MAX_IO_PICS];
>>   extern struct irq_domain *pch_pic_domain[MAX_IO_PICS];
>>   
>> diff --git a/arch/loongarch/kernel/irq.c b/arch/loongarch/kernel/irq.c
>> index b34b8d7..07d6059 100644
>> --- a/arch/loongarch/kernel/irq.c
>> +++ b/arch/loongarch/kernel/irq.c
>> @@ -27,7 +27,6 @@
>>   
>>   struct irq_domain *cpu_domain;
>>   struct irq_domain *liointc_domain;
>> -struct irq_domain *pch_lpc_domain;
>>   struct irq_domain *pch_msi_domain[MAX_IO_PICS];
>>   struct irq_domain *pch_pic_domain[MAX_IO_PICS];
>>   
>> diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig
>> index 1f23a6b..c1d527f 100644
>> --- a/drivers/irqchip/Kconfig
>> +++ b/drivers/irqchip/Kconfig
>> @@ -591,6 +591,14 @@ config LOONGSON_PCH_MSI
>>   	help
>>   	  Support for the Loongson PCH MSI Controller.
>>   
>> +config LOONGSON_PCH_LPC
>> +	bool "Loongson PCH LPC Controller"
>> +	depends on MACH_LOONGSON64
>> +	default (MACH_LOONGSON64 && LOONGARCH)
>> +	select IRQ_DOMAIN_HIERARCHY
>> +	help
>> +	  Support for the Loongson PCH LPC Controller.
>> +
>>   config MST_IRQ
>>   	bool "MStar Interrupt Controller"
>>   	depends on ARCH_MEDIATEK || ARCH_MSTARV7 || COMPILE_TEST
>> diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
>> index 5b67450..242b8b3 100644
>> --- a/drivers/irqchip/Makefile
>> +++ b/drivers/irqchip/Makefile
>> @@ -108,6 +108,7 @@ obj-$(CONFIG_LOONGSON_HTPIC)		+= irq-loongson-htpic.o
>>   obj-$(CONFIG_LOONGSON_HTVEC)		+= irq-loongson-htvec.o
>>   obj-$(CONFIG_LOONGSON_PCH_PIC)		+= irq-loongson-pch-pic.o
>>   obj-$(CONFIG_LOONGSON_PCH_MSI)		+= irq-loongson-pch-msi.o
>> +obj-$(CONFIG_LOONGSON_PCH_LPC)		+= irq-loongson-pch-lpc.o
>>   obj-$(CONFIG_MST_IRQ)			+= irq-mst-intc.o
>>   obj-$(CONFIG_SL28CPLD_INTC)		+= irq-sl28cpld.o
>>   obj-$(CONFIG_MACH_REALTEK_RTL)		+= irq-realtek-rtl.o
>> diff --git a/drivers/irqchip/irq-loongson-pch-lpc.c b/drivers/irqchip/irq-loongson-pch-lpc.c
>> new file mode 100644
>> index 0000000..7e4d89a
>> --- /dev/null
>> +++ b/drivers/irqchip/irq-loongson-pch-lpc.c
>> @@ -0,0 +1,203 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Loongson LPC Interrupt Controller support
>> + *
>> + * Copyright (C) 2020-2022 Loongson Technology Corporation Limited
>> + */
>> +
>> +#define pr_fmt(fmt) "lpc: " fmt
>> +
>> +#include <linux/interrupt.h>
>> +#include <linux/irq.h>
>> +#include <linux/irqchip.h>
>> +#include <linux/irqchip/chained_irq.h>
>> +#include <linux/irqdomain.h>
>> +#include <linux/kernel.h>
>> +
>> +/* Registers */
>> +#define LPC_INT_CTL		0x00
>> +#define LPC_INT_ENA		0x04
>> +#define LPC_INT_STS		0x08
>> +#define LPC_INT_CLR		0x0c
>> +#define LPC_INT_POL		0x10
>> +#define LPC_COUNT		16
>> +
>> +struct pch_lpc {
>> +	void __iomem		*base;
>> +	struct irq_domain	*lpc_domain;
>> +	raw_spinlock_t		lpc_lock;
>> +	u32			saved_reg_ctl;
>> +	u32			saved_reg_ena;
>> +	u32			saved_reg_pol;
>> +};
>> +
>> +struct fwnode_handle *pch_lpc_handle;
>> +
>> +static void ack_lpc_irq(struct irq_data *d)
>> +{
>> +	unsigned long flags;
>> +	struct pch_lpc *priv = d->domain->host_data;
>> +
>> +	raw_spin_lock_irqsave(&priv->lpc_lock, flags);
>> +	writel(0x1 << d->hwirq, priv->base + LPC_INT_CLR);
>> +	raw_spin_unlock_irqrestore(&priv->lpc_lock, flags);
>> +}
>> +static void mask_lpc_irq(struct irq_data *d)
>> +{
>> +	unsigned long flags;
>> +	struct pch_lpc *priv = d->domain->host_data;
>> +
>> +	raw_spin_lock_irqsave(&priv->lpc_lock, flags);
>> +	writel(readl(priv->base + LPC_INT_ENA) & (~(0x1 << (d->hwirq))),
>> +			priv->base + LPC_INT_ENA);
>> +	raw_spin_unlock_irqrestore(&priv->lpc_lock, flags);
>> +}
>> +
>> +static void unmask_lpc_irq(struct irq_data *d)
>> +{
>> +	unsigned long flags;
>> +	struct pch_lpc *priv = d->domain->host_data;
>> +
>> +	raw_spin_lock_irqsave(&priv->lpc_lock, flags);
>> +	writel(readl(priv->base + LPC_INT_ENA) | (0x1 << (d->hwirq)),
>> +			priv->base + LPC_INT_ENA);
>> +	raw_spin_unlock_irqrestore(&priv->lpc_lock, flags);
>> +}
>> +
>> +static int lpc_set_type(struct irq_data *d, unsigned int type)
>> +{
>> +	u32 val;
>> +	u32 mask = 0x1 << (d->hwirq);
>> +	struct pch_lpc *priv = d->domain->host_data;
>> +
>> +	if (!(type & IRQ_TYPE_LEVEL_MASK))
>> +		return 0;
>> +
>> +	val = readl(priv->base + LPC_INT_POL);
>> +
>> +	if (type == IRQ_TYPE_LEVEL_HIGH)
>> +		val |= mask;
>> +	else
>> +		val &= ~mask;
>> +
>> +	writel(val, priv->base + LPC_INT_POL);
>> +
>> +	return 0;
>> +}
>> +
>> +static struct irq_chip pch_lpc_irq_chip = {
> 
> Make this const.
> 

Ok, thanks.


>> +	.name			= "PCH LPC",
>> +	.irq_mask		= mask_lpc_irq,
>> +	.irq_unmask		= unmask_lpc_irq,
>> +	.irq_ack		= ack_lpc_irq,
>> +	.irq_set_type		= lpc_set_type,
> 
> nit: please use the same format for all callbacks: lpc_irq_OPERATION
> (lpc_irq_mask, lpc_irq_set_type...), which makes it much easier to
> read.
> 

Ok.


>> +	.flags			= IRQCHIP_SKIP_SET_WAKE,
>> +};
>> +
>> +static void lpc_irq_dispatch(struct irq_desc *desc)
>> +{
>> +	u32 pending, bit, virq;
>> +	struct irq_chip *chip = irq_desc_get_chip(desc);
>> +	struct pch_lpc *priv = irq_desc_get_handler_data(desc);
>> +
>> +	chained_irq_enter(chip, desc);
>> +
>> +	pending = readl(priv->base + LPC_INT_ENA);
>> +	pending &= readl(priv->base + LPC_INT_STS);
>> +	if (!pending)
>> +		spurious_interrupt();
>> +
>> +	while (pending) {
>> +		bit = __ffs(pending);
>> +		virq = irq_linear_revmap(priv->lpc_domain, bit);
>> +
>> +		generic_handle_irq(bit);
> 
> Replace the whole thing with a call to generic_handle_domain_irq(). No
> new code should ever have to use generic_handle_irq().
> 

Ok, thanks, got it.


>> +		pending &= ~BIT(bit);
>> +	}
>> +	chained_irq_exit(chip, desc);
>> +}
>> +
>> +static int pch_lpc_map(struct irq_domain *d, unsigned int irq,
>> +			irq_hw_number_t hw)
>> +{
>> +	irq_set_chip_and_handler(irq, &pch_lpc_irq_chip, handle_level_irq);
>> +	return 0;
>> +}
>> +
>> +static const struct irq_domain_ops pch_lpc_domain_ops = {
>> +	.map 		= pch_lpc_map,
>> +	.translate	= irq_domain_translate_twocell,
>> +};
>> +
>> +static void pch_lpc_reset(struct pch_lpc *priv)
>> +{
>> +	/* Enable the LPC interrupt, bit31: en  bit30: edge */
>> +	writel(0x80000000, priv->base + LPC_INT_CTL);
> 
> Please don't hardcode numbers. Add the proper bit definitions.
> 

Ok.


>> +	writel(0, priv->base + LPC_INT_ENA);
>> +	/* Clear all 18-bit interrpt bit */
>> +	writel(0x3ffff, priv->base + LPC_INT_CLR);
> 
> If that's just a mask, then use GENMASK(17, 0) instead.
> 

Ok.


>> +}
>> +
>> +static int pch_lpc_disabled(struct pch_lpc *priv)
>> +{
>> +	return (readl(priv->base + LPC_INT_ENA) == 0xffffffff) &&
>> +			(readl(priv->base + LPC_INT_STS) == 0xffffffff);
>> +}
>> +
>> +int __init pch_lpc_acpi_init(struct irq_domain *parent,
>> +					struct acpi_madt_lpc_pic *acpi_pchlpc)
>> +{
>> +	int parent_irq;
>> +	struct pch_lpc *priv;
>> +	struct irq_fwspec fwspec;
>> +	struct fwnode_handle *irq_handle;
>> +
>> +	if (!acpi_pchlpc)
>> +		return -EINVAL;
>> +
>> +	priv = kzalloc(sizeof(*priv), GFP_KERNEL);
>> +	if (!priv)
>> +		return -ENOMEM;
>> +
>> +	raw_spin_lock_init(&priv->lpc_lock);
>> +
>> +	priv->base = ioremap(acpi_pchlpc->address, acpi_pchlpc->size);
>> +	if (!priv->base)
>> +		goto free_priv;
>> +
>> +	if (pch_lpc_disabled(priv)) {
>> +		pr_err("Failed to get LPC status\n");
>> +		goto iounmap_base;
>> +	}
>> +
>> +	irq_handle = irq_domain_alloc_named_fwnode("lpcintc");
>> +	if (!irq_handle) {
>> +		pr_err("Unable to allocate domain handle\n");
>> +		goto iounmap_base;
>> +	}
>> +
>> +	priv->lpc_domain = irq_domain_create_linear(irq_handle, LPC_COUNT,
>> +					&pch_lpc_domain_ops, priv);
>> +	if (!priv->lpc_domain) {
>> +		pr_err("Failed to create IRQ domain\n");
>> +		goto iounmap_base;
> 
> You also need to free the fwnode.
> 

Yes, thanks.

>> +	}
>> +	pch_lpc_reset(priv);
>> +
>> +	fwspec.fwnode = parent->fwnode;
>> +	fwspec.param[0] = acpi_pchlpc->cascade;
>> +	fwspec.param[1] = IRQ_TYPE_LEVEL_HIGH;
>> +	fwspec.param_count = 2;
>> +	parent_irq = irq_create_fwspec_mapping(&fwspec);
>> +	irq_set_chained_handler_and_data(parent_irq, lpc_irq_dispatch, priv);
>> +
>> +	pch_lpc_handle = irq_handle;
>> +	return 0;
>> +
>> +iounmap_base:
>> +	iounmap(priv->base);
>> +free_priv:
>> +	kfree(priv);
>> +
>> +	return -ENOMEM;
>> +}
>> -- 
>> 1.8.3.1
>>
>>
> 
> Thanks,
> 
> 	M.
>