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

[Thomas Gleixner]

On Mon, Jul 22 2024 at 11:41, ysionneau@xxxxxxxxxxxxx wrote:
> +
> +static void kvx_itgen_mask(struct irq_data *data)
> +{
> +	kvx_itgen_enable(data, 0x0);
> +	irq_chip_mask_parent(data);
> +}
> +
> +static void kvx_itgen_unmask(struct irq_data *data)
> +{
> +	kvx_itgen_enable(data, 0x1);
> +	irq_chip_unmask_parent(data);

The ordering is asymmetric vs. mask().

> +#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);

So you warn here and then continue without fixing type up?

> +	err = platform_msi_device_domain_alloc(domain, virq, nr_irqs);

This function is gone as of 6.11-rc1. Please convert to parent MSI domain.

> +	if (err)
> +		return err;
> +

> +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);

Please create proper inline functions or macros for this 

> +	cfg_val |= ((dest_addr >> MB_ADDR_MAILBOX_SHIFT) &
> +		     KVX_ITGEN_CFG_TARGET_MAILBOX_MASK)
> +		    << KVX_ITGEN_CFG_TARGET_MAILBOX_SHIFT;

and this.

> +	/*
> +	 * 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
> +kvx_itgen_device_probe(struct platform_device *pdev)

Pointless line break.

> +{
> +	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);
> +	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);

This function is gone as of 6.11-rc1. Please convert to parent MSI domain.

Thanks,

        tglx