On Mon, Jan 29 2024 at 21:58, Changhuang Liang wrote:
> +
> +struct starfive_irq_chip {
> + void __iomem *base;
> + struct irq_domain *root_domain;
> + struct clk *clk;
> +};
https://www.kernel.org/doc/html/latest/process/maintainer-tip.html#struct-declarations-and-initializers
Please.
> +static void starfive_intc_mod(struct starfive_irq_chip *irqc, u32 reg, u32 mask, u32 data)
> +{
> + u32 value;
> +
> + value = ioread32(irqc->base + reg) & ~mask;
> + data &= mask;
Why?
> + data |= value;
> + iowrite32(data, irqc->base + reg);
How is this serialized against concurrent invocations of this code on
different CPUs for different interrupts?
It's not and this requires a raw_spinlock for protection.
> +}
> +
> +static void starfive_intc_unmask(struct irq_data *d)
> +{
> + struct starfive_irq_chip *irqc = irq_data_get_irq_chip_data(d);
> +
> + starfive_intc_mod(irqc, STARFIVE_INTC_SRC0_MASK, BIT(d->hwirq), 0);
> +}
> +
> +static void starfive_intc_mask(struct irq_data *d)
> +{
> + struct starfive_irq_chip *irqc = irq_data_get_irq_chip_data(d);
> +
> + starfive_intc_mod(irqc, STARFIVE_INTC_SRC0_MASK, BIT(d->hwirq), BIT(d->hwirq));
> +}
> +
> +static struct irq_chip intc_dev = {
> + .name = "starfive jh8100 intc",
> + .irq_unmask = starfive_intc_unmask,
> + .irq_mask = starfive_intc_mask,
> +};
See documentation please.
> +static int starfive_intc_map(struct irq_domain *d, unsigned int irq, irq_hw_number_t hwirq)
> +{
> + irq_domain_set_info(d, irq, hwirq, &intc_dev, d->host_data,
> + handle_level_irq, NULL, NULL);
> +
> + return 0;
> +}
> +
> +static const struct irq_domain_ops starfive_intc_domain_ops = {
> + .xlate = irq_domain_xlate_onecell,
> + .map = starfive_intc_map,
> +};
Ditto.
> +static void starfive_intc_irq_handler(struct irq_desc *desc)
> +{
> + struct irq_chip *chip = irq_desc_get_chip(desc);
> + struct starfive_irq_chip *irqc = irq_data_get_irq_handler_data(&desc->irq_data);
https://www.kernel.org/doc/html/latest/process/maintainer-tip.html#variable-declarations
> + unsigned long value = 0;
Pointless initialization
> + int hwirq;
> +
> + chained_irq_enter(chip, desc);
> +
> + value = ioread32(irqc->base + STARFIVE_INTC_SRC0_INT);
> + while (value) {
> + hwirq = ffs(value) - 1;
> +
> + generic_handle_domain_irq(irqc->root_domain, hwirq);
> +
> + starfive_intc_mod(irqc, STARFIVE_INTC_SRC0_CLEAR, BIT(hwirq), BIT(hwirq));
> + starfive_intc_mod(irqc, STARFIVE_INTC_SRC0_CLEAR, BIT(hwirq), 0);
> +
> + clear_bit(hwirq, &value);
> + }
> +
> + chained_irq_exit(chip, desc);
> +}
> +
> +static int __init starfive_intc_init(struct device_node *intc,
> + struct device_node *parent)
> +{
> + struct starfive_irq_chip *irqc;
> + struct reset_control *rst;
> + int ret;
> + int parent_irq;
See Documentation
> + irqc = kzalloc(sizeof(*irqc), GFP_KERNEL);
> + if (!irqc)
> + return -ENOMEM;
> +
> + irqc->base = of_iomap(intc, 0);
> + if (!irqc->base) {
> + pr_err("Unable to map IC registers\n");
> + ret = -ENXIO;
> + goto err_free;
> + }
> +
> + rst = of_reset_control_get_exclusive(intc, NULL);
> + if (IS_ERR(rst)) {
> + pr_err("Unable to get reset control %pe\n", rst);
> + ret = PTR_ERR(rst);
> + goto err_unmap;
> + }
> +
> + irqc->clk = of_clk_get(intc, 0);
> + if (IS_ERR(irqc->clk)) {
> + pr_err("Unable to get clock\n");
> + ret = PTR_ERR(irqc->clk);
> + goto err_rst;
> + }
> +
> + ret = reset_control_deassert(rst);
> + if (ret)
> + goto err_clk;
> +
> + ret = clk_prepare_enable(irqc->clk);
> + if (ret)
> + goto err_clk;
> +
> + irqc->root_domain = irq_domain_add_linear(intc, STARFIVE_INTC_SRC_IRQ_NUM,
> + &starfive_intc_domain_ops, irqc);
> + if (!irqc->root_domain) {
> + pr_err("Unable to create IRQ domain\n");
> + ret = -EINVAL;
> + goto err_clk;
> + }
> +
> + parent_irq = of_irq_get(intc, 0);
> + if (parent_irq < 0) {
> + pr_err("Failed to get main IRQ: %d\n", parent_irq);
> + ret = parent_irq;
> + goto err_clk;
Leaks the interrupt domain, no?
Thanks,
tglx
