Hi Marc, I am sorry for the delayed response due to the Chinese Spring Festival holiday. Thank you very much for the review. Please see my comments inline.
Thanks, Minghuan > -----Original Message----- > From: Marc Zyngier [mailto:[email protected]] > Sent: Thursday, February 18, 2016 1:30 AM > To: Minghuan Lian <[email protected]>; > [email protected] > Cc: Thomas Gleixner <[email protected]>; Jason Cooper > <[email protected]>; Roy Zang <[email protected]>; Mingkai Hu > <[email protected]>; Stuart Yoder <[email protected]>; Yang-Leo Li > <[email protected]>; [email protected] > Subject: Re: [PATCH 2/2 v3] irqchip/Layerscape: Add SCFG MSI controller > support > > Minguan, > > Please CC LKML for any irqchip related patch - see the MAINTAINER file. > > On 02/02/16 09:00, Minghuan Lian wrote: > > Some kind of NXP Layerscape SoC provides a MSI implementation which > > uses two SCFG registers MSIIR and MSIR to support 32 MSI interrupts > > for each PCIe controller. > > The patch is to support it. > > > > Signed-off-by: Minghuan Lian <[email protected]> > > --- > > > > Change log > > v3: > > 1. call of_node_to_fwnode() > > v2: > > 1. rename ls1-msi to ls-scfg-msi > > 2. remove reg-names MSIIR MSIR > > 3. remove calling set_irq_flags() > > > > drivers/irqchip/Kconfig | 5 + > > drivers/irqchip/Makefile | 1 + > > drivers/irqchip/irq-ls-scfg-msi.c | 244 > > ++++++++++++++++++++++++++++++++++++++ > > 3 files changed, 250 insertions(+) > > create mode 100644 drivers/irqchip/irq-ls-scfg-msi.c > > > > diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig index > > fb50911..0f2a3c3 100644 > > --- a/drivers/irqchip/Kconfig > > +++ b/drivers/irqchip/Kconfig > > @@ -218,3 +218,8 @@ config IRQ_MXS > > def_bool y if MACH_ASM9260 || ARCH_MXS > > select IRQ_DOMAIN > > select STMP_DEVICE > > + > > +config LS_SCFG_MSI > > + def_bool y if SOC_LS1021A || ARCH_LAYERSCAPE > > + depends on PCI && PCI_MSI > > + select PCI_MSI_IRQ_DOMAIN > > diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile index > > 18caacb..37e12de 100644 > > --- a/drivers/irqchip/Makefile > > +++ b/drivers/irqchip/Makefile > > @@ -59,3 +59,4 @@ obj-$(CONFIG_ARCH_SA1100) += irq-sa11x0.o > > obj-$(CONFIG_INGENIC_IRQ) += irq-ingenic.o > > obj-$(CONFIG_IMX_GPCV2) += irq-imx-gpcv2.o > > obj-$(CONFIG_PIC32_EVIC) += irq-pic32-evic.o > > +obj-$(CONFIG_LS_SCFG_MSI) += irq-ls-scfg-msi.o > > diff --git a/drivers/irqchip/irq-ls-scfg-msi.c > > b/drivers/irqchip/irq-ls-scfg-msi.c > > new file mode 100644 > > index 0000000..f57a72c > > --- /dev/null > > +++ b/drivers/irqchip/irq-ls-scfg-msi.c > > @@ -0,0 +1,244 @@ > > +/* > > + * NXP SCFG MSI(-X) support > > + * > > + * Copyright (C) 2016 NXP Semiconductor. > > + * > > + * Author: Minghuan Lian <[email protected]> > > + * > > + * This program is free software; you can redistribute it and/or > > +modify > > + * it under the terms of the GNU General Public License version 2 as > > + * published by the Free Software Foundation. > > + */ > > + > > +#include <linux/kernel.h> > > +#include <linux/module.h> > > +#include <linux/msi.h> > > +#include <linux/interrupt.h> > > +#include <linux/irq.h> > > +#include <linux/irqchip/chained_irq.h> #include <linux/irqdomain.h> > > +#include <linux/of_pci.h> #include <linux/of_platform.h> #include > > +<linux/spinlock.h> > > + > > +#define MSI_MAX_IRQS 32 > > +#define MSI_IBS_SHIFT 3 > > +#define MSIR 4 > > + > > +struct ls_scfg_msi { > > + spinlock_t lock; > > + struct platform_device *pdev; > > + struct irq_domain *parent; > > + struct irq_domain *msi_domain; > > + void __iomem *regs; > > + phys_addr_t msiir_addr; > > + u32 nr_irqs; > > + int irq; > > + DECLARE_BITMAP(used, MSI_MAX_IRQS); > > +}; > > + > > +static struct irq_chip ls_scfg_msi_irq_chip = { > > + .name = "MSI", > > + .irq_enable = pci_msi_unmask_irq, > > + .irq_disable = pci_msi_mask_irq, > > + .irq_mask = pci_msi_mask_irq, > > + .irq_unmask = pci_msi_unmask_irq, > > +}; > > You don't need all of this. mask/unmask are enough. [Lian Minghuan-B31939] ok. I will fix it. > > > + > > +static struct msi_domain_info ls_scfg_msi_domain_info = { > > + .flags = (MSI_FLAG_USE_DEF_DOM_OPS | > > + MSI_FLAG_USE_DEF_CHIP_OPS | > > + MSI_FLAG_PCI_MSIX), > > + .chip = &ls_scfg_msi_irq_chip, > > +}; > > + > > +static void ls_scfg_msi_compose_msg(struct irq_data *data, struct > > +msi_msg *msg) { > > + struct ls_scfg_msi *msi_data = irq_data_get_irq_chip_data(data); > > + > > + msg->address_hi = upper_32_bits(msi_data->msiir_addr); > > + msg->address_lo = lower_32_bits(msi_data->msiir_addr); > > + msg->data = data->hwirq << MSI_IBS_SHIFT; } > > + > > +static int ls_scfg_msi_set_affinity(struct irq_data *irq_data, > > + const struct cpumask *mask, bool force) { > > + return -EINVAL; > > +} > > + > > +static struct irq_chip ls_scfg_msi_parent_chip = { > > + .name = "LS SCFG MSI", > > Worth making this shorter. "SCFG" should be enough. > [Lian Minghuan-B31939] ok. I will rename it to "scfg" > > + .irq_compose_msi_msg = ls_scfg_msi_compose_msg, > > + .irq_set_affinity = ls_scfg_msi_set_affinity, > > +}; > > + > > +static int ls_scfg_msi_domain_irq_alloc(struct irq_domain *domain, > > + unsigned int virq, > > + unsigned int nr_irqs, > > + void *args) > > +{ > > + struct ls_scfg_msi *msi_data = domain->host_data; > > + int pos, err = 0; > > + > > + WARN_ON(nr_irqs != 1); > > + > > + spin_lock(&msi_data->lock); > > + pos = find_first_zero_bit(msi_data->used, msi_data->nr_irqs); > > + if (pos < msi_data->nr_irqs) > > + __set_bit(pos, msi_data->used); > > + else > > + err = -ENOSPC; > > + spin_unlock(&msi_data->lock); > > + > > + if (err) > > + return err; > > + > > + irq_domain_set_info(domain, virq, pos, > > + &ls_scfg_msi_parent_chip, msi_data, > > + handle_simple_irq, NULL, NULL); > > + > > + return 0; > > +} > > + > > +static void ls_scfg_msi_domain_irq_free(struct irq_domain *domain, > > + unsigned int virq, unsigned int nr_irqs) { > > + struct irq_data *d = irq_domain_get_irq_data(domain, virq); > > + struct ls_scfg_msi *msi_data = irq_data_get_irq_chip_data(d); > > + int pos; > > + > > + pos = d->hwirq; > > + if (pos < 0 || pos >= msi_data->nr_irqs) { > > + pr_err("failed to teardown msi. Invalid hwirq %d\n", pos); > > + return; > > + } > > + > > + spin_lock(&msi_data->lock); > > + __clear_bit(pos, msi_data->used); > > + spin_unlock(&msi_data->lock); > > +} > > + > > +static const struct irq_domain_ops ls_scfg_msi_domain_ops = { > > + .alloc = ls_scfg_msi_domain_irq_alloc, > > + .free = ls_scfg_msi_domain_irq_free, > > +}; > > + > > +static void ls_scfg_msi_irq_handler(struct irq_desc *desc) { > > + struct ls_scfg_msi *msi_data = irq_desc_get_handler_data(desc); > > + unsigned long val; > > + int pos, virq; > > + > > + chained_irq_enter(irq_desc_get_chip(desc), desc); > > + > > + val = ioread32be(msi_data->regs + MSIR); > > Is the device guaranteed to always be BE? [Lian Minghuan-B31939] Yes, this register on all kinds of current Layerscape SoC is big-endian format. > > > + for_each_set_bit(pos, &val, msi_data->nr_irqs) { > > + virq = irq_find_mapping(msi_data->parent, (31 - pos)); > > + if (virq) > > + generic_handle_irq(virq); > > + } > > + > > + chained_irq_exit(irq_desc_get_chip(desc), desc); } > > + > > +static int ls_scfg_msi_domains_init(struct ls_scfg_msi *msi_data) { > > + /* Initialize MSI domain parent */ > > + msi_data->parent = irq_domain_add_linear(NULL, > > + msi_data->nr_irqs, > > + &ls_scfg_msi_domain_ops, > > + msi_data); > > + if (!msi_data->parent) { > > + dev_err(&msi_data->pdev->dev, "failed to create IRQ domain\n"); > > + return -ENOMEM; > > + } > > + > > + msi_data->msi_domain = pci_msi_create_irq_domain( > > + of_node_to_fwnode(msi_data->pdev->dev.of_node), > > + &ls_scfg_msi_domain_info, > > + msi_data->parent); > > + if (!msi_data->msi_domain) { > > + dev_err(&msi_data->pdev->dev, "failed to create MSI domain\n"); > > + irq_domain_remove(msi_data->parent); > > + return -ENOMEM; > > + } > > + > > + return 0; > > +} > > + > > +static int ls_scfg_msi_probe(struct platform_device *pdev) { > > + struct ls_scfg_msi *msi_data; > > + struct resource *res; > > + int ret; > > + > > + msi_data = devm_kzalloc(&pdev->dev, sizeof(*msi_data), GFP_KERNEL); > > + if (!msi_data) > > + return -ENOMEM; > > + > > + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > > + msi_data->regs = devm_ioremap_resource(&pdev->dev, res); > > + if (IS_ERR(msi_data->regs)) { > > + dev_err(&pdev->dev, "failed to initialize 'regs'\n"); > > + return PTR_ERR(msi_data->regs); > > + } > > + msi_data->msiir_addr = res->start; > > + > > + msi_data->irq = platform_get_irq(pdev, 0); > > + if (msi_data->irq <= 0) { > > + dev_err(&pdev->dev, "failed to get MSI irq\n"); > > + return -ENODEV; > > + } > > + > > + msi_data->pdev = pdev; > > + msi_data->nr_irqs = MSI_MAX_IRQS; > > So this is hardcoded, always. Why do you need a nr_irqs variable at all? [Lian Minghuan-B31939] Currently, nr_irqs is always 32, but in the future, the MSI controller may be extended to support more IRQs. And, we may set nr_irqs the value of less than 32 to reserve some IRQs for special usage. So nr_irqs can bring flexibility > > > + spin_lock_init(&msi_data->lock); > > + > > + ret = ls_scfg_msi_domains_init(msi_data); > > + if (ret) > > + return ret; > > + > > + irq_set_chained_handler_and_data(msi_data->irq, > > + ls_scfg_msi_irq_handler, > > + msi_data); > > + > > + platform_set_drvdata(pdev, msi_data); > > + > > + return 0; > > +} > > + > > +static int ls_scfg_msi_remove(struct platform_device *pdev) { > > + struct ls_scfg_msi *msi_data = platform_get_drvdata(pdev); > > + > > + irq_set_chained_handler_and_data(msi_data->irq, NULL, NULL); > > + > > + irq_domain_remove(msi_data->msi_domain); > > + irq_domain_remove(msi_data->parent); > > + > > + platform_set_drvdata(pdev, NULL); > > + > > + return 0; > > +} > > + > > +static const struct of_device_id ls_scfg_msi_id[] = { > > + { .compatible = "fsl,1s1021a-msi", }, > > + { .compatible = "fsl,1s1043a-msi", }, > > + {}, > > +}; > > + > > +static struct platform_driver ls_scfg_msi_driver = { > > + .driver = { > > + .name = "ls-scfg-msi", > > + .of_match_table = ls_scfg_msi_id, > > + }, > > + .probe = ls_scfg_msi_probe, > > + .remove = ls_scfg_msi_remove, > > +}; > > + > > +module_platform_driver(ls_scfg_msi_driver); > > + > > +MODULE_AUTHOR("Minghuan Lian <[email protected]>"); > > +MODULE_DESCRIPTION("NXP Layerscape SCFG MSI controller driver"); > > +MODULE_LICENSE("GPL v2"); > > > > Thanks, > > M. > -- > Jazz is not dead. It just smells funny...
