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
