Re: [RFC PATCH v3 20/37] irqchip: Add irq-kvx-itgen driver

Krzysztof Kozlowski
2024-07-22T12:30:59Z

On 22/07/2024 11:41, ysionneau@kalrayinc.com wrote:
> From: Yann Sionneau <ysionneau@kalrayinc.com>
>
> The Kalray Coolidge SoC contains several interrupt generators (itgen).
> The itgen is both an interrupt-controller and a msi-client.
>
> Peripheral Controllers such as PCIe, I2C, SPI, GPIO, etc.
> need to send interrupts to the Compute Clusters.
> The purpose of this module is to forward interrupts to the compute clusters
> through the AXI interconnect.

...

> +#define ITGEN_UNSUPPORTED_TYPES (IRQ_TYPE_LEVEL_LOW | IRQ_TYPE_EDGE_FALLING)
> +
> +static int kvx_itgen_domain_alloc(struct irq_domain *domain, unsigned int virq,
> + unsigned int nr_irqs, void *args)
> +{
> + int i, err;
> + struct irq_fwspec *fwspec = args;
> + int hwirq = fwspec->param[0];
> + int type = IRQ_TYPE_NONE;
> + struct kvx_itgen *itgen;
> +
> + if (fwspec->param_count >= 2)
> + type = fwspec->param[1];
> +
> + WARN_ON(type & ITGEN_UNSUPPORTED_TYPES);

How is this possible?

> +

> +
> +static void kvx_itgen_write_msg(struct msi_desc *desc, struct msi_msg *msg)
> +{
> + struct irq_data *d = irq_get_irq_data(desc->irq);
> + struct kvx_itgen *itgen = irq_data_get_irq_chip_data(d);
> + uint32_t cfg_val = 0;
> + uintptr_t dest_addr = ((uint64_t) msg->address_hi << 32) |
> + msg->address_lo;
> + void __iomem *cfg = get_itgen_cfg_offset(itgen, irqd_to_hwirq(d));
> +
> + /*
> + * The address passed in the msi data is the address of the target
> + * mailbox. The itgen however writes to the mailbox based on the mppa
> + * id, cluster id and mailbox id instead of an address. So, extract
> + * these information from the mailbox address.
> + */
> +
> + cfg_val |= (((kvx_sfr_get(PCR) & KVX_SFR_PCR_CID_MASK) >>
> + KVX_SFR_PCR_CID_SHIFT)
> + << KVX_ITGEN_CFG_TARGET_CLUSTER_SHIFT);
> + cfg_val |= ((dest_addr >> MB_ADDR_MAILBOX_SHIFT) &
> + KVX_ITGEN_CFG_TARGET_MAILBOX_MASK)
> + << KVX_ITGEN_CFG_TARGET_MAILBOX_SHIFT;
> +
> + /*
> + * msg->data contains the bit number to be written and is included in
> + * the itgen config
> + */
> + cfg_val |= ((msg->data << KVX_ITGEN_CFG_TARGET_SELECT_BIT_SHIFT)
> + & KVX_ITGEN_CFG_TARGET_SELECT_BIT_MASK);
> +
> + dev_dbg(&itgen->pdev->dev,
> + "Writing dest_addr %lx, value %x to cfg %p\n",
> + dest_addr, cfg_val, cfg);
> +
> + writel(cfg_val, cfg);
> +}
> +
> +static int

Why is this wrapped? Does not look like exceeding 80.

> +kvx_itgen_device_probe(struct platform_device *pdev)
> +{
> + struct kvx_itgen *itgen;
> + u32 it_count;
> + struct resource *mem;
> +
> + itgen = devm_kzalloc(&pdev->dev, sizeof(*itgen), GFP_KERNEL);
> + if (!itgen)
> + return -ENOMEM;
> +
> + mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + itgen->base = devm_ioremap_resource(&pdev->dev, mem);

Use proper wrapper over these two.

> + if (IS_ERR(itgen->base))
> + return dev_err_probe(&pdev->dev, PTR_ERR(itgen->base),
> + "Failed to ioremap itgen\n");
> +
> + itgen->pdev = pdev;
> + it_count = readl(get_itgen_param_offset(itgen) +
> + KVX_ITGEN_PARAM_IT_NUM_OFFSET);
> +
> + itgen->domain = platform_msi_create_device_domain(&pdev->dev,
> + it_count,
> + kvx_itgen_write_msg,
> + &itgen_domain_ops,
> + itgen);
> + if (!itgen->domain) {
> + dev_err(&pdev->dev, "Failed to create device domain\n");
> + return -ENOMEM;
> + }
> +
> + platform_set_drvdata(pdev, itgen);
> +
> + return 0;
> +}
> +
> +static const struct of_device_id itgen_of_match[] = {
> + { .compatible = "kalray,coolidge-itgen" },
> + { /* END */ }

Drop comment, that's really obvious.

Best regards,
Krzysztof