Message-ID: <krieplgqalbjs7o5ylv5t6x77csuoknnhyc2pzdga4tu4mu73w@vjux46t3ijxr>
Date: Mon, 7 Oct 2024 08:38:53 +0800
From: Inochi Amaoto <inochiama@...il.com>
To: Thomas Gleixner <tglx@...utronix.de>, 
	Inochi Amaoto <inochiama@...il.com>, Chen Wang <unicorn_wang@...look.com>, 
	Rob Herring <robh@...nel.org>, Krzysztof Kozlowski <krzk+dt@...nel.org>, 
	Conor Dooley <conor+dt@...nel.org>, Paul Walmsley <paul.walmsley@...ive.com>, 
	Palmer Dabbelt <palmer@...belt.com>, Albert Ou <aou@...s.berkeley.edu>, 
	Peter Zijlstra <peterz@...radead.org>, Inochi Amaoto <inochiama@...look.com>, 
	Guo Ren <guoren@...nel.org>, Lad Prabhakar <prabhakar.mahadev-lad.rj@...renesas.com>, 
	Hal Feng <hal.feng@...rfivetech.com>, Heikki Krogerus <heikki.krogerus@...ux.intel.com>, 
	Geert Uytterhoeven <geert+renesas@...der.be>
Cc: Yixun Lan <dlan@...too.org>, linux-kernel@...r.kernel.org, 
	devicetree@...r.kernel.org, linux-riscv@...ts.infradead.org
Subject: Re: [PATCH 2/3] irqchip: add T-HEAD C900 ACLINT SSWI driver

On Sun, Oct 06, 2024 at 09:50:39PM +0200, Thomas Gleixner wrote:
> On Fri, Oct 04 2024 at 16:05, Inochi Amaoto wrote:
> 
> > +#define pr_fmt(fmt) "thead-c900-aclint-sswi: " fmt
> > +#include <linux/acpi.h>
> 
> What is this header used for?
> 

This is copy-pasted error, I wiil remove it.

> > +static void thead_aclint_sswi_ipi_clear(void)
> > +{
> > +	unsigned int cpu = smp_processor_id();
> > +	struct aclint_sswi_cpu_config *config = per_cpu_ptr(&sswi_cpus, cpu);
> 
> That's an unnecessary indirection.
> 
>        *config = __this_cpu_ptr(&sswi_cpus);
> 
> is what you want here.

Thanks.

> 
> > +	writel_relaxed(0x0, config->reg + config->offset);
> > +}
> 
> ...
> 
> > +static int aclint_sswi_parse_irq(struct fwnode_handle *fwnode,
> > +				 void __iomem *reg)
> 
> Please avoid line breaks and use up to 100 characters per line.
> 
> > +{
> > +	struct of_phandle_args parent;
> > +	unsigned long hartid;
> > +	u32 contexts, i;
> > +	int rc, cpu;
> > +	struct aclint_sswi_cpu_config *config;
> 
> https://www.kernel.org/doc/html/latest/process/maintainer-tip.html#variable-declarations
> 
> > +
> > +	contexts = of_irq_count(to_of_node(fwnode));
> > +	if (WARN_ON(!(contexts))) {
> 
> That WARN_ON() is pointless. The call chain is known and the pr_err() is
> sufficient.
> 
> > +		pr_err("%pfwP: no ACLINT SSWI context available\n", fwnode);
> > +		return -EINVAL;
> > +	}
> > +
> > +	for (i = 0; i < contexts; i++) {
> > +		rc = of_irq_parse_one(to_of_node(fwnode), i, &parent);
> > +		if (rc)
> > +			return rc;
> > +
> > +		rc = riscv_of_parent_hartid(parent.np, &hartid);
> > +		if (rc)
> > +			return rc;
> > +
> > +		if (parent.args[0] != RV_IRQ_SOFT)
> > +			return -ENOTSUPP;
> > +
> > +		cpu = riscv_hartid_to_cpuid(hartid);
> > +		config = per_cpu_ptr(&sswi_cpus, cpu);
> > +
> > +		config->offset = i * ACLINT_xSWI_REGISTER_SIZE;
> > +		config->reg = reg;
> 
> Why do you need config->reg and config->offset? All call sites access
> the register via:
> 
>     config->reg + config->offset
> 
> So you can spare the exercise of adding the offset in the hotpath by
> adding it at setup time, no?

Thanks, I only consider supporting multiple device, but forgot that it
can be computed earily.

> 
> 
> > +	}
> > +
> > +	pr_info("%pfwP: register %u CPU\n", fwnode, contexts);
> 
>   ...CPU%s\n", fwnode, contexts, str_plural(contexts));
> 
> > +
> > +	return 0;
> > +}
> > +
> > +static int __init aclint_sswi_probe(struct fwnode_handle *fwnode)
> > +{
> > +	void __iomem *reg;
> > +	struct irq_domain *domain;
> > +	int virq, rc;
> 
> See above.
> 
> > +	if (!is_of_node(fwnode))
> > +		return -EINVAL;
> > +
> > +	reg = of_iomap(to_of_node(fwnode), 0);
> > +	if (!reg)
> > +		return -ENOMEM;
> > +
> > +	/* Parse SSWI setting */
> > +	rc = aclint_sswi_parse_irq(fwnode, reg);
> > +	if (rc < 0)
> > +		return rc;
> > +
> > +	/* If mulitple SSWI devices are present, do not register irq again */
> > +	if (sswi_ipi_virq)
> > +		return 0;
> > +
> > +	/* Find and create irq domain */
> 
> Which domain is created here?
> 

It will create an IPI domain. I will update the comment.

> > +	domain = irq_find_matching_fwnode(riscv_get_intc_hwnode(), DOMAIN_BUS_ANY);
> > +	if (!domain) {
> > +		pr_err("%pfwP: Failed to find INTC domain\n", fwnode);
> > +		return -ENOENT;
> > +	}
> > +
> > +	sswi_ipi_virq = irq_create_mapping(domain, RV_IRQ_SOFT);
> > +	if (!sswi_ipi_virq) {
> > +		pr_err("unable to create ACLINT SSWI IRQ mapping\n");
> > +		return -ENOMEM;
> > +	}
> > +
> > +	/* Register SSWI irq and handler */
> > +	virq = ipi_mux_create(BITS_PER_BYTE, thead_aclint_sswi_ipi_send);
> > +	if (virq <= 0) {
> > +		pr_err("unable to create muxed IPIs\n");
> > +		irq_dispose_mapping(sswi_ipi_virq);
> > +		return virq < 0 ? virq : -ENOMEM;
> > +	}
> > +
> > +	irq_set_chained_handler(sswi_ipi_virq, thead_aclint_sswi_ipi_handle);
> > +
> > +	cpuhp_setup_state(CPUHP_AP_IRQ_THEAD_ACLINT_SSWI_STARTING,
> > +			  "irqchip/thead-aclint-sswi:starting",
> > +			  aclint_sswi_ipi_starting_cpu, NULL);
> 
> The startup callback enables the per CPU interrupt. When a CPU is
> offlined then the per CPU interrupt stays enabled because the teardown
> callback is NULL. I'm not convinced that this is a good idea.
> 

Yes, I will add the cleanup handle to clear IPI and disable the IPI 
irq for the CPU.

> > +
> > +	riscv_ipi_set_virq_range(virq, BITS_PER_BYTE);
> > +
> > +	/* Announce that SSWI is providing IPIs */
> > +	pr_info("providing IPIs using THEAD ACLINT SSWI\n");
> > +
> > +	return 0;
> > +}
> > +
> > +static int __init aclint_sswi_early_probe(struct device_node *node,
> > +					  struct device_node *parent)
> > +{
> > +	return aclint_sswi_probe(&node->fwnode);
> > +}
> 
> What's the point of this indirection?
> 

This is make room for the future ACPI probe.

> > +
> 
> Pointless newline.
> 
> > +IRQCHIP_DECLARE(thead_aclint_sswi, "thead,c900-aclint-sswi", aclint_sswi_early_probe);
> 
> Thanks,
> 
>         tglx

Regards,
Inochi

Powered by blists - more mailing lists