[PATCH 2/4] irqchip: irq-armada-370-xp: use the generic MSI infrastructure

[marc.zyngier@arm.com (Marc Zyngier)]

Hi Thomas,

I finally found some bandwidth to have a look at this. A few comments below:

On 21/12/15 14:19, Thomas Petazzoni wrote:
> This commit moves the irq-armada-370-xp driver from using the
> PCI-specific MSI infrastructure to the generic MSI infrastructure, to
> which drivers are progressively converted.
> 
> In this hardware, the MSI controller is directly bundled inside the
> interrupt controller, so we have a single Device Tree node to which
> multiple IRQ domaines are attached: the wired interrupt domain and the
> MSI interrupt domain. In order to ensure that they can be
> differentiated, we have to force the bus_token of the wired interrupt
> domain to be DOMAIN_BUS_WIRED. The MSI domain bus_token is
> automatically set to the appropriate value by
> pci_msi_create_irq_domain().
> 
> Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
> Suggested-by: Marc Zyngier <marc.zyngier@arm.com>
> ---
>  drivers/irqchip/Kconfig             |   1 +
>  drivers/irqchip/irq-armada-370-xp.c | 151 +++++++++++++++---------------------
>  2 files changed, 65 insertions(+), 87 deletions(-)
> 
> diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig
> index ab672b0..8ccb60e 100644
> --- a/drivers/irqchip/Kconfig
> +++ b/drivers/irqchip/Kconfig
> @@ -51,6 +51,7 @@ config ARMADA_370_XP_IRQ
>  	bool
>  	default y if ARCH_MVEBU
>  	select GENERIC_IRQ_CHIP
> +	select PCI_MSI_IRQ_DOMAIN if PCI_MSI
>  
>  config ATMEL_AIC_IRQ
>  	bool
> diff --git a/drivers/irqchip/irq-armada-370-xp.c b/drivers/irqchip/irq-armada-370-xp.c
> index 3f3a8c3..304166b 100644
> --- a/drivers/irqchip/irq-armada-370-xp.c
> +++ b/drivers/irqchip/irq-armada-370-xp.c
> @@ -71,6 +71,7 @@ static u32 doorbell_mask_reg;
>  static int parent_irq;
>  #ifdef CONFIG_PCI_MSI
>  static struct irq_domain *armada_370_xp_msi_domain;
> +static struct irq_domain *armada_370_xp_msi_inner_domain;
>  static DECLARE_BITMAP(msi_used, PCI_MSI_DOORBELL_NR);
>  static DEFINE_MUTEX(msi_used_lock);
>  static phys_addr_t msi_doorbell_addr;
> @@ -115,127 +116,102 @@ static void armada_370_xp_irq_unmask(struct irq_data *d)
>  
>  #ifdef CONFIG_PCI_MSI
>  
> -static int armada_370_xp_alloc_msi(void)
> -{
> -	int hwirq;
> -
> -	mutex_lock(&msi_used_lock);
> -	hwirq = find_first_zero_bit(&msi_used, PCI_MSI_DOORBELL_NR);
> -	if (hwirq >= PCI_MSI_DOORBELL_NR)
> -		hwirq = -ENOSPC;
> -	else
> -		set_bit(hwirq, msi_used);
> -	mutex_unlock(&msi_used_lock);
> +static struct irq_chip armada_370_xp_msi_irq_chip = {
> +	.name = "armada_370_xp_msi_irq",
> +	.irq_enable = pci_msi_unmask_irq,
> +	.irq_disable = pci_msi_mask_irq,
> +	.irq_mask = pci_msi_mask_irq,
> +	.irq_unmask = pci_msi_unmask_irq,

Having only mask/unmask ought to be enough.

> +};
>  
> -	return hwirq;
> -}
> +static struct msi_domain_info armada_370_xp_msi_domain_info = {
> +	.flags	= (MSI_FLAG_USE_DEF_DOM_OPS | MSI_FLAG_USE_DEF_CHIP_OPS |
> +		   MSI_FLAG_MULTI_PCI_MSI),

And not MSI-X? That seems rather strange (I'm pretty sure it just works).

> +	.chip	= &armada_370_xp_msi_irq_chip,
> +};
>  
> -static void armada_370_xp_free_msi(int hwirq)
> +static void armada_370_xp_compose_msi_msg(struct irq_data *data, struct msi_msg *msg)
>  {
> -	mutex_lock(&msi_used_lock);
> -	if (!test_bit(hwirq, msi_used))
> -		pr_err("trying to free unused MSI#%d\n", hwirq);
> -	else
> -		clear_bit(hwirq, msi_used);
> -	mutex_unlock(&msi_used_lock);
> +	msg->address_lo = lower_32_bits(msi_doorbell_addr);
> +	msg->address_hi = upper_32_bits(msi_doorbell_addr);
> +	msg->data = 0xf00 | (data->hwirq + PCI_MSI_DOORBELL_START);
>  }
>  
> -static int armada_370_xp_setup_msi_irq(struct msi_controller *chip,
> -				       struct pci_dev *pdev,
> -				       struct msi_desc *desc)
> +static int armada_370_xp_msi_set_affinity(struct irq_data *irq_data,
> +					  const struct cpumask *mask, bool force)
>  {
> -	struct msi_msg msg;
> -	int virq, hwirq;
> +	 return -EINVAL;
> +}
>  
> -	/* We support MSI, but not MSI-X */
> -	if (desc->msi_attrib.is_msix)
> -		return -EINVAL;
> +static struct irq_chip armada_370_xp_msi_bottom_irq_chip = {
> +	.name			= "MPIC MSI",
> +	.irq_compose_msi_msg	= armada_370_xp_compose_msi_msg,
> +	.irq_set_affinity	= armada_370_xp_msi_set_affinity,
> +};
>  
> -	hwirq = armada_370_xp_alloc_msi();
> -	if (hwirq < 0)
> -		return hwirq;
> +static int armada_370_xp_msi_alloc(struct irq_domain *domain, unsigned int virq,
> +				   unsigned int nr_irqs, void *args)
> +{
> +	int hwirq;
>  
> -	virq = irq_create_mapping(armada_370_xp_msi_domain, hwirq);
> -	if (!virq) {
> -		armada_370_xp_free_msi(hwirq);
> -		return -EINVAL;
> +	mutex_lock(&msi_used_lock);
> +	hwirq = find_first_zero_bit(&msi_used, PCI_MSI_DOORBELL_NR);
> +	if (hwirq >= PCI_MSI_DOORBELL_NR) {
> +		mutex_unlock(&msi_used_lock);
> +		return -ENOSPC;
>  	}
>  
> -	irq_set_msi_desc(virq, desc);
> +	set_bit(hwirq, msi_used);
> +	mutex_unlock(&msi_used_lock);
>  
> -	msg.address_lo = msi_doorbell_addr;
> -	msg.address_hi = 0;
> -	msg.data = 0xf00 | (hwirq + 16);
> +	irq_domain_set_info(domain, virq, hwirq, &armada_370_xp_msi_bottom_irq_chip,
> +			    domain->host_data, handle_simple_irq,
> +			    NULL, NULL);
>  
> -	pci_write_msi_msg(virq, &msg);
> -	return 0;
> +	return hwirq;

So you are allocating one bit at a time, irrespective of nr_irqs. This
implies that you can't support MULTI_MSI here (you'd need to guarantee a
contiguous allocation of nr_irqs bits). So either you amend your
allocator to deal with those (pretty easy), or you drop MULTI_MSI from
your msi_domain_info.

>  }
>  
> -static void armada_370_xp_teardown_msi_irq(struct msi_controller *chip,
> -					   unsigned int irq)
> +static void armada_370_xp_msi_free(struct irq_domain *domain,
> +				   unsigned int virq, unsigned int nr_irqs)
>  {
> -	struct irq_data *d = irq_get_irq_data(irq);
> -	unsigned long hwirq = d->hwirq;
> +	struct irq_data *d = irq_domain_get_irq_data(domain, virq);
>  
> -	irq_dispose_mapping(irq);
> -	armada_370_xp_free_msi(hwirq);
> -}
> -
> -static struct irq_chip armada_370_xp_msi_irq_chip = {
> -	.name = "armada_370_xp_msi_irq",
> -	.irq_enable = pci_msi_unmask_irq,
> -	.irq_disable = pci_msi_mask_irq,
> -	.irq_mask = pci_msi_mask_irq,
> -	.irq_unmask = pci_msi_unmask_irq,
> -};
> -
> -static int armada_370_xp_msi_map(struct irq_domain *domain, unsigned int virq,
> -				 irq_hw_number_t hw)
> -{
> -	irq_set_chip_and_handler(virq, &armada_370_xp_msi_irq_chip,
> -				 handle_simple_irq);
> -
> -	return 0;
> +	mutex_lock(&msi_used_lock);
> +	if (!test_bit(d->hwirq, msi_used))
> +		pr_err("trying to free unused MSI#%lu\n", d->hwirq);
> +	else
> +		clear_bit(d->hwirq, msi_used);
> +	mutex_unlock(&msi_used_lock);
>  }
>  
> -static const struct irq_domain_ops armada_370_xp_msi_irq_ops = {
> -	.map = armada_370_xp_msi_map,
> +static const struct irq_domain_ops armada_370_xp_msi_domain_ops = {
> +	.alloc	= armada_370_xp_msi_alloc,
> +	.free	= armada_370_xp_msi_free,
>  };
>  
>  static int armada_370_xp_msi_init(struct device_node *node,
>  				  phys_addr_t main_int_phys_base)
>  {
> -	struct msi_controller *msi_chip;
>  	u32 reg;
> -	int ret;
>  
>  	msi_doorbell_addr = main_int_phys_base +
>  		ARMADA_370_XP_SW_TRIG_INT_OFFS;
>  
> -	msi_chip = kzalloc(sizeof(*msi_chip), GFP_KERNEL);
> -	if (!msi_chip)
> +	armada_370_xp_msi_inner_domain =
> +		irq_domain_add_linear(NULL, PCI_MSI_DOORBELL_NR,
> +				      &armada_370_xp_msi_domain_ops, NULL);

nit: Please keep the assignment on a single line.

> +	if (!armada_370_xp_msi_inner_domain)
>  		return -ENOMEM;
>  
> -	msi_chip->setup_irq = armada_370_xp_setup_msi_irq;
> -	msi_chip->teardown_irq = armada_370_xp_teardown_msi_irq;
> -	msi_chip->of_node = node;
> -
>  	armada_370_xp_msi_domain =
> -		irq_domain_add_linear(NULL, PCI_MSI_DOORBELL_NR,
> -				      &armada_370_xp_msi_irq_ops,
> -				      NULL);
> +		pci_msi_create_irq_domain(of_node_to_fwnode(node),
> +					  &armada_370_xp_msi_domain_info,
> +					  armada_370_xp_msi_inner_domain);

Same here.

>  	if (!armada_370_xp_msi_domain) {
> -		kfree(msi_chip);
> +		irq_domain_remove(armada_370_xp_msi_inner_domain);
>  		return -ENOMEM;
>  	}
>  
> -	ret = of_pci_msi_chip_add(msi_chip);
> -	if (ret < 0) {
> -		irq_domain_remove(armada_370_xp_msi_domain);
> -		kfree(msi_chip);
> -		return ret;
> -	}
> -
>  	reg = readl(per_cpu_int_base + ARMADA_370_XP_IN_DRBEL_MSK_OFFS)
>  		| PCI_MSI_DOORBELL_MASK;
>  
> @@ -427,12 +403,12 @@ static void armada_370_xp_handle_msi_irq(struct pt_regs *regs, bool is_chained)
>  			continue;
>  
>  		if (is_chained) {
> -			irq = irq_find_mapping(armada_370_xp_msi_domain,
> +			irq = irq_find_mapping(armada_370_xp_msi_inner_domain,
>  					       msinr - 16);
>  			generic_handle_irq(irq);
>  		} else {
>  			irq = msinr - 16;
> -			handle_domain_irq(armada_370_xp_msi_domain,
> +			handle_domain_irq(armada_370_xp_msi_inner_domain,
>  					  irq, regs);
>  		}
>  	}
> @@ -604,6 +580,7 @@ static int __init armada_370_xp_mpic_of_init(struct device_node *node,
>  	armada_370_xp_mpic_domain =
>  		irq_domain_add_linear(node, nr_irqs,
>  				&armada_370_xp_mpic_irq_ops, NULL);
> +	armada_370_xp_mpic_domain->bus_token = DOMAIN_BUS_WIRED;
>  
>  	BUG_ON(!armada_370_xp_mpic_domain);

Given the assignment just above, this BUG_ON has become pretty useless...

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...