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
