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

Yann Sionneau
2024-08-23T12:45:04Z

Hello Krzysztof,

On 22/07/2024 14:30, Krzysztof Kozlowski wrote:
> On 22/07/2024 11:41, ysionneau@kalrayinc.com wrote:
> ...
>
>> +#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?

If DT contains something like this:

interrupt-parent = <&itgen IRQ_TYPE_LEVEL_LOW>;

Then this is not supported.

But I reckon we can replace WARN_ON() with pr_warn() to avoid crashing machine with "panic on warn" setting.

>> +
>> +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.
Ack, I will fix it.
>> +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.

You mean I should call devm_platform_get_and_ioremap_resource <https://elixir.bootlin.com/linux/v6.10.3/C/ident/devm_platform_get_and_ioremap_resource>()?

Ok I will do this.

...

> +
> +static const struct of_device_id itgen_of_match[] = {
> + { .compatible = "kalray,coolidge-itgen" },
> + { /* END */ }
> Drop comment, that's really obvious.

Ack, I will do this.

Regards,

--

Yann Sionneau