Re: [PATCH 2/3] irqchip: add T-HEAD C900 ACLINT SSWI driver

[Thomas Gleixner <tglx@linutronix.de>]

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?

> +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.

> +	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?


> +	}
> +
> +	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?

> +	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.

> +
> +	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?

> +

Pointless newline.

> +IRQCHIP_DECLARE(thead_aclint_sswi, "thead,c900-aclint-sswi", aclint_sswi_early_probe);

Thanks,

        tglx