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

[Krzysztof Kozlowski]

On 22/07/2024 11:41, ysionneau@xxxxxxxxxxxxx wrote:
> From: Yann Sionneau <ysionneau@xxxxxxxxxxxxx>
> 
> 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