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

[Yann Sionneau]

Hello Krzysztof,

On 22/07/2024 14:30, Krzysztof Kozlowski wrote:
> On 22/07/2024 11:41, ysionneau@xxxxxxxxxxxxx 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