On Thu, Feb 25, 2021 at 03:10:13PM +0000, Marc Zyngier wrote:
> In anticipation of the removal of the msi_controller structure, convert
> the ancient xilinx host controller driver to MSI domains.
>
> We end-up with the usual two domain structure, the top one being a
> generic PCI/MSI domain, the bottom one being xilinx-specific and handling
> the actual HW interrupt allocation.
>
> This allows us to fix some of the most appaling MSI programming, where
> the message programmed in the device is the virtual IRQ number instead
> of the allocated vector number. The allocator is also made safe with
> a mutex. This should allow support for MultiMSI, but I decided not to
> even try, since I cannot test it.
>
> Also take the opportunity to get rid of the cargo-culted memory allocation
> for the MSI capture address. *ANY* sufficiently aligned address should
> be good enough, so use the physical address of the xilinx_pcie_host
> structure instead.
>
> Signed-off-by: Marc Zyngier <m...@kernel.org>
> ---
> drivers/pci/controller/Kconfig | 2 +-
> drivers/pci/controller/pcie-xilinx.c | 238 +++++++++++----------------
> 2 files changed, 96 insertions(+), 144 deletions(-)
Michal,
can you please test these changes or make sure someone does and report
back on the mailing list please ?
I would like to merge this series for v5.13.
Thanks,
Lorenzo
>
> diff --git a/drivers/pci/controller/Kconfig b/drivers/pci/controller/Kconfig
> index ccbf034512d6..049c60016904 100644
> --- a/drivers/pci/controller/Kconfig
> +++ b/drivers/pci/controller/Kconfig
> @@ -95,7 +95,7 @@ config PCI_HOST_GENERIC
> config PCIE_XILINX
> bool "Xilinx AXI PCIe host bridge support"
> depends on OF || COMPILE_TEST
> - select PCI_MSI_ARCH_FALLBACKS
> + depends on PCI_MSI_IRQ_DOMAIN
> help
> Say 'Y' here if you want kernel to support the Xilinx AXI PCIe
> Host Bridge driver.
> diff --git a/drivers/pci/controller/pcie-xilinx.c
> b/drivers/pci/controller/pcie-xilinx.c
> index fa5baeb82653..ad9abf405167 100644
> --- a/drivers/pci/controller/pcie-xilinx.c
> +++ b/drivers/pci/controller/pcie-xilinx.c
> @@ -93,25 +93,23 @@
> /**
> * struct xilinx_pcie_port - PCIe port information
> * @reg_base: IO Mapped Register Base
> - * @irq: Interrupt number
> - * @msi_pages: MSI pages
> * @dev: Device pointer
> + * @msi_map: Bitmap of allocated MSIs
> + * @map_lock: Mutex protecting the MSI allocation
> * @msi_domain: MSI IRQ domain pointer
> * @leg_domain: Legacy IRQ domain pointer
> * @resources: Bus Resources
> */
> struct xilinx_pcie_port {
> void __iomem *reg_base;
> - u32 irq;
> - unsigned long msi_pages;
> struct device *dev;
> + unsigned long msi_map[BITS_TO_LONGS(XILINX_NUM_MSI_IRQS)];
> + struct mutex map_lock;
> struct irq_domain *msi_domain;
> struct irq_domain *leg_domain;
> struct list_head resources;
> };
>
> -static DECLARE_BITMAP(msi_irq_in_use, XILINX_NUM_MSI_IRQS);
> -
> static inline u32 pcie_read(struct xilinx_pcie_port *port, u32 reg)
> {
> return readl(port->reg_base + reg);
> @@ -196,151 +194,108 @@ static struct pci_ops xilinx_pcie_ops = {
>
> /* MSI functions */
>
> -/**
> - * xilinx_pcie_destroy_msi - Free MSI number
> - * @irq: IRQ to be freed
> - */
> -static void xilinx_pcie_destroy_msi(unsigned int irq)
> +static struct irq_chip xilinx_msi_top_chip = {
> + .name = "PCIe MSI",
> +};
> +
> +static int xilinx_msi_set_affinity(struct irq_data *d, const struct cpumask
> *mask, bool force)
> {
> - struct msi_desc *msi;
> - struct xilinx_pcie_port *port;
> - struct irq_data *d = irq_get_irq_data(irq);
> - irq_hw_number_t hwirq = irqd_to_hwirq(d);
> -
> - if (!test_bit(hwirq, msi_irq_in_use)) {
> - msi = irq_get_msi_desc(irq);
> - port = msi_desc_to_pci_sysdata(msi);
> - dev_err(port->dev, "Trying to free unused MSI#%d\n", irq);
> - } else {
> - clear_bit(hwirq, msi_irq_in_use);
> - }
> + return -EINVAL;
> }
>
> -/**
> - * xilinx_pcie_assign_msi - Allocate MSI number
> - *
> - * Return: A valid IRQ on success and error value on failure.
> - */
> -static int xilinx_pcie_assign_msi(void)
> +static void xilinx_compose_msi_msg(struct irq_data *data, struct msi_msg
> *msg)
> {
> - int pos;
> + struct xilinx_pcie_port *pcie = irq_data_get_irq_chip_data(data);
> + phys_addr_t pa = virt_to_phys(pcie);
>
> - pos = find_first_zero_bit(msi_irq_in_use, XILINX_NUM_MSI_IRQS);
> - if (pos < XILINX_NUM_MSI_IRQS)
> - set_bit(pos, msi_irq_in_use);
> - else
> - return -ENOSPC;
> -
> - return pos;
> + msg->address_lo = lower_32_bits(pa);
> + msg->address_hi = upper_32_bits(pa);
> + msg->data = data->hwirq;
> }
>
> -/**
> - * xilinx_msi_teardown_irq - Destroy the MSI
> - * @chip: MSI Chip descriptor
> - * @irq: MSI IRQ to destroy
> - */
> -static void xilinx_msi_teardown_irq(struct msi_controller *chip,
> - unsigned int irq)
> -{
> - xilinx_pcie_destroy_msi(irq);
> - irq_dispose_mapping(irq);
> -}
> +static struct irq_chip xilinx_msi_bottom_chip = {
> + .name = "Xilinx MSI",
> + .irq_set_affinity = xilinx_msi_set_affinity,
> + .irq_compose_msi_msg = xilinx_compose_msi_msg,
> +};
>
> -/**
> - * xilinx_pcie_msi_setup_irq - Setup MSI request
> - * @chip: MSI chip pointer
> - * @pdev: PCIe device pointer
> - * @desc: MSI descriptor pointer
> - *
> - * Return: '0' on success and error value on failure
> - */
> -static int xilinx_pcie_msi_setup_irq(struct msi_controller *chip,
> - struct pci_dev *pdev,
> - struct msi_desc *desc)
> +static int xilinx_msi_domain_alloc(struct irq_domain *domain, unsigned int
> virq,
> + unsigned int nr_irqs, void *args)
> {
> - struct xilinx_pcie_port *port = pdev->bus->sysdata;
> - unsigned int irq;
> - int hwirq;
> - struct msi_msg msg;
> - phys_addr_t msg_addr;
> + struct xilinx_pcie_port *port = domain->host_data;
> + int hwirq, i;
>
> - hwirq = xilinx_pcie_assign_msi();
> - if (hwirq < 0)
> - return hwirq;
> -
> - irq = irq_create_mapping(port->msi_domain, hwirq);
> - if (!irq)
> - return -EINVAL;
> + mutex_lock(&port->map_lock);
>
> - irq_set_msi_desc(irq, desc);
> + hwirq = bitmap_find_free_region(port->msi_map, XILINX_NUM_MSI_IRQS,
> order_base_2(nr_irqs));
>
> - msg_addr = virt_to_phys((void *)port->msi_pages);
> + mutex_unlock(&port->map_lock);
>
> - msg.address_hi = 0;
> - msg.address_lo = msg_addr;
> - msg.data = irq;
> + if (hwirq < 0)
> + return -ENOSPC;
>
> - pci_write_msi_msg(irq, &msg);
> + for (i = 0; i < nr_irqs; i++)
> + irq_domain_set_info(domain, virq + i, hwirq + i,
> + &xilinx_msi_bottom_chip, domain->host_data,
> + handle_edge_irq, NULL, NULL);
>
> return 0;
> }
>
> -/* MSI Chip Descriptor */
> -static struct msi_controller xilinx_pcie_msi_chip = {
> - .setup_irq = xilinx_pcie_msi_setup_irq,
> - .teardown_irq = xilinx_msi_teardown_irq,
> -};
> +static void xilinx_msi_domain_free(struct irq_domain *domain, unsigned int
> virq,
> + unsigned int nr_irqs)
> +{
> + struct irq_data *d = irq_domain_get_irq_data(domain, virq);
> + struct xilinx_pcie_port *port = domain->host_data;
>
> -/* HW Interrupt Chip Descriptor */
> -static struct irq_chip xilinx_msi_irq_chip = {
> - .name = "Xilinx PCIe 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,
> -};
> + mutex_lock(&port->map_lock);
>
> -/**
> - * xilinx_pcie_msi_map - Set the handler for the MSI and mark IRQ as valid
> - * @domain: IRQ domain
> - * @irq: Virtual IRQ number
> - * @hwirq: HW interrupt number
> - *
> - * Return: Always returns 0.
> - */
> -static int xilinx_pcie_msi_map(struct irq_domain *domain, unsigned int irq,
> - irq_hw_number_t hwirq)
> -{
> - irq_set_chip_and_handler(irq, &xilinx_msi_irq_chip, handle_simple_irq);
> - irq_set_chip_data(irq, domain->host_data);
> + bitmap_release_region(port->msi_map, d->hwirq, order_base_2(nr_irqs));
>
> - return 0;
> + mutex_unlock(&port->map_lock);
> }
>
> -/* IRQ Domain operations */
> -static const struct irq_domain_ops msi_domain_ops = {
> - .map = xilinx_pcie_msi_map,
> +static const struct irq_domain_ops xilinx_msi_domain_ops = {
> + .alloc = xilinx_msi_domain_alloc,
> + .free = xilinx_msi_domain_free,
> };
>
> -/**
> - * xilinx_pcie_enable_msi - Enable MSI support
> - * @port: PCIe port information
> - */
> -static int xilinx_pcie_enable_msi(struct xilinx_pcie_port *port)
> +static struct msi_domain_info xilinx_msi_info = {
> + .flags = (MSI_FLAG_USE_DEF_DOM_OPS | MSI_FLAG_USE_DEF_CHIP_OPS),
> + .chip = &xilinx_msi_top_chip,
> +};
> +
> +static int xilinx_allocate_msi_domains(struct xilinx_pcie_port *pcie)
> {
> - phys_addr_t msg_addr;
> + struct fwnode_handle *fwnode = dev_fwnode(pcie->dev);
> + struct irq_domain *parent;
>
> - port->msi_pages = __get_free_pages(GFP_KERNEL, 0);
> - if (!port->msi_pages)
> + parent = irq_domain_create_linear(fwnode, XILINX_NUM_MSI_IRQS,
> + &xilinx_msi_domain_ops, pcie);
> + if (!parent) {
> + dev_err(pcie->dev, "failed to create IRQ domain\n");
> return -ENOMEM;
> + }
> + irq_domain_update_bus_token(parent, DOMAIN_BUS_NEXUS);
>
> - msg_addr = virt_to_phys((void *)port->msi_pages);
> - pcie_write(port, 0x0, XILINX_PCIE_REG_MSIBASE1);
> - pcie_write(port, msg_addr, XILINX_PCIE_REG_MSIBASE2);
> + pcie->msi_domain = pci_msi_create_irq_domain(fwnode, &xilinx_msi_info,
> parent);
> + if (!pcie->msi_domain) {
> + dev_err(pcie->dev, "failed to create MSI domain\n");
> + irq_domain_remove(parent);
> + return -ENOMEM;
> + }
>
> return 0;
> }
>
> +static void xilinx_free_msi_domains(struct xilinx_pcie_port *pcie)
> +{
> + struct irq_domain *parent = pcie->msi_domain->parent;
> +
> + irq_domain_remove(pcie->msi_domain);
> + irq_domain_remove(parent);
> +}
> +
> /* INTx Functions */
>
> /**
> @@ -420,6 +375,8 @@ static irqreturn_t xilinx_pcie_intr_handler(int irq, void
> *data)
> }
>
> if (status & (XILINX_PCIE_INTR_INTX | XILINX_PCIE_INTR_MSI)) {
> + unsigned int irq;
> +
> val = pcie_read(port, XILINX_PCIE_REG_RPIFR1);
>
> /* Check whether interrupt valid */
> @@ -432,20 +389,19 @@ static irqreturn_t xilinx_pcie_intr_handler(int irq,
> void *data)
> if (val & XILINX_PCIE_RPIFR1_MSI_INTR) {
> val = pcie_read(port, XILINX_PCIE_REG_RPIFR2) &
> XILINX_PCIE_RPIFR2_MSG_DATA;
> + irq = irq_find_mapping(port->msi_domain->parent, val);
> } else {
> val = (val & XILINX_PCIE_RPIFR1_INTR_MASK) >>
> XILINX_PCIE_RPIFR1_INTR_SHIFT;
> - val = irq_find_mapping(port->leg_domain, val);
> + irq = irq_find_mapping(port->leg_domain, val);
> }
>
> /* Clear interrupt FIFO register 1 */
> pcie_write(port, XILINX_PCIE_RPIFR1_ALL_MASK,
> XILINX_PCIE_REG_RPIFR1);
>
> - /* Handle the interrupt */
> - if (IS_ENABLED(CONFIG_PCI_MSI) ||
> - !(val & XILINX_PCIE_RPIFR1_MSI_INTR))
> - generic_handle_irq(val);
> + if (irq)
> + generic_handle_irq(irq);
> }
>
> if (status & XILINX_PCIE_INTR_SLV_UNSUPP)
> @@ -491,12 +447,11 @@ static irqreturn_t xilinx_pcie_intr_handler(int irq,
> void *data)
> static int xilinx_pcie_init_irq_domain(struct xilinx_pcie_port *port)
> {
> struct device *dev = port->dev;
> - struct device_node *node = dev->of_node;
> struct device_node *pcie_intc_node;
> int ret;
>
> /* Setup INTx */
> - pcie_intc_node = of_get_next_child(node, NULL);
> + pcie_intc_node = of_get_next_child(dev->of_node, NULL);
> if (!pcie_intc_node) {
> dev_err(dev, "No PCIe Intc node found\n");
> return -ENODEV;
> @@ -513,18 +468,14 @@ static int xilinx_pcie_init_irq_domain(struct
> xilinx_pcie_port *port)
>
> /* Setup MSI */
> if (IS_ENABLED(CONFIG_PCI_MSI)) {
> - port->msi_domain = irq_domain_add_linear(node,
> - XILINX_NUM_MSI_IRQS,
> - &msi_domain_ops,
> - &xilinx_pcie_msi_chip);
> - if (!port->msi_domain) {
> - dev_err(dev, "Failed to get a MSI IRQ domain\n");
> - return -ENODEV;
> - }
> + phys_addr_t pa = virt_to_phys(port);
>
> - ret = xilinx_pcie_enable_msi(port);
> + ret = xilinx_allocate_msi_domains(port);
> if (ret)
> return ret;
> +
> + pcie_write(port, upper_32_bits(pa), XILINX_PCIE_REG_MSIBASE1);
> + pcie_write(port, lower_32_bits(pa), XILINX_PCIE_REG_MSIBASE2);
> }
>
> return 0;
> @@ -572,6 +523,7 @@ static int xilinx_pcie_parse_dt(struct xilinx_pcie_port
> *port)
> struct device *dev = port->dev;
> struct device_node *node = dev->of_node;
> struct resource regs;
> + unsigned int irq;
> int err;
>
> err = of_address_to_resource(node, 0, ®s);
> @@ -584,12 +536,12 @@ static int xilinx_pcie_parse_dt(struct xilinx_pcie_port
> *port)
> if (IS_ERR(port->reg_base))
> return PTR_ERR(port->reg_base);
>
> - port->irq = irq_of_parse_and_map(node, 0);
> - err = devm_request_irq(dev, port->irq, xilinx_pcie_intr_handler,
> + irq = irq_of_parse_and_map(node, 0);
> + err = devm_request_irq(dev, irq, xilinx_pcie_intr_handler,
> IRQF_SHARED | IRQF_NO_THREAD,
> "xilinx-pcie", port);
> if (err) {
> - dev_err(dev, "unable to request irq %d\n", port->irq);
> + dev_err(dev, "unable to request irq %d\n", irq);
> return err;
> }
>
> @@ -617,7 +569,7 @@ static int xilinx_pcie_probe(struct platform_device *pdev)
> return -ENODEV;
>
> port = pci_host_bridge_priv(bridge);
> -
> + mutex_init(&port->map_lock);
> port->dev = dev;
>
> err = xilinx_pcie_parse_dt(port);
> @@ -637,11 +589,11 @@ static int xilinx_pcie_probe(struct platform_device
> *pdev)
> bridge->sysdata = port;
> bridge->ops = &xilinx_pcie_ops;
>
> -#ifdef CONFIG_PCI_MSI
> - xilinx_pcie_msi_chip.dev = dev;
> - bridge->msi = &xilinx_pcie_msi_chip;
> -#endif
> - return pci_host_probe(bridge);
> + err = pci_host_probe(bridge);
> + if (err)
> + xilinx_free_msi_domains(port);
> +
> + return err;
> }
>
> static const struct of_device_id xilinx_pcie_of_match[] = {
> --
> 2.29.2
>
