Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1607468 > unrolled thread

[RFC PATCH v0.2] PCI: Add support for tango PCIe host bridge

Back to article view | Back to linux.kernel

Contents

  [RFC PATCH v0.2] PCI: Add support for tango PCIe host bridge Mason <slash.tmp@free.fr> - 2017-03-23 14:10 +0100
    Re: [RFC PATCH v0.2] PCI: Add support for tango PCIe host bridge Marc Zyngier <marc.zyngier@arm.com> - 2017-03-23 15:30 +0100
      Re: [RFC PATCH v0.2] PCI: Add support for tango PCIe host bridge Mason <slash.tmp@free.fr> - 2017-03-23 18:10 +0100
        Re: [RFC PATCH v0.2] PCI: Add support for tango PCIe host bridge Mason <slash.tmp@free.fr> - 2017-03-24 00:50 +0100
          Re: [RFC PATCH v0.2] PCI: Add support for tango PCIe host bridge Marc Zyngier <marc.zyngier@arm.com> - 2017-03-24 19:30 +0100
            Re: [RFC PATCH v0.2] PCI: Add support for tango PCIe host bridge Mason <slash.tmp@free.fr> - 2017-03-27 16:40 +0200
              Re: [RFC PATCH v0.2] PCI: Add support for tango PCIe host bridge Thomas Gleixner <tglx@linutronix.de> - 2017-03-27 16:50 +0200
                Re: [RFC PATCH v0.2] PCI: Add support for tango PCIe host bridge Mason <slash.tmp@free.fr> - 2017-03-27 17:20 +0200
        Re: [RFC PATCH v0.2] PCI: Add support for tango PCIe host bridge Marc Zyngier <marc.zyngier@arm.com> - 2017-03-24 19:50 +0100
          Re: [RFC PATCH v0.2] PCI: Add support for tango PCIe host bridge Mason <slash.tmp@free.fr> - 2017-03-27 18:10 +0200

#1607468 — [RFC PATCH v0.2] PCI: Add support for tango PCIe host bridge

I think this version is ready for review.
It has all the required bits and pieces.
I still have a few questions, embedded as comments in the code.
(Missing are ancillary changes to Kconfig, Makefile)
---
 drivers/pci/host/pcie-tango.c | 350 ++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 350 insertions(+)
 create mode 100644 drivers/pci/host/pcie-tango.c

diff --git a/drivers/pci/host/pcie-tango.c b/drivers/pci/host/pcie-tango.c
new file mode 100644
index 000000000000..b2e6448aed2d
--- /dev/null
+++ b/drivers/pci/host/pcie-tango.c
@@ -0,0 +1,350 @@
+#include <linux/irqchip/chained_irq.h>
+#include <linux/irqdomain.h>
+#include <linux/pci-ecam.h>
+#include <linux/msi.h>
+
+#define MSI_COUNT 32
+
+struct tango_pcie {
+	void __iomem *mux;
+	void __iomem *msi_status;
+	void __iomem *msi_mask;
+	phys_addr_t msi_doorbell;
+	struct mutex lock; /* lock for updating msi_mask */
+	struct irq_domain *irq_domain;
+	struct irq_domain *msi_domain;
+	int irq;
+};
+
+/*** MSI CONTROLLER SUPPORT ***/
+
+static void tango_msi_isr(struct irq_desc *desc)
+{
+	struct irq_chip *chip = irq_desc_get_chip(desc);
+	struct tango_pcie *pcie;
+	unsigned long status, virq;
+	int pos;
+
+	chained_irq_enter(chip, desc);
+	pcie = irq_desc_get_handler_data(desc);
+
+	status = readl_relaxed(pcie->msi_status);
+	writel_relaxed(status, pcie->msi_status); /* clear IRQs */
+
+	for_each_set_bit(pos, &status, MSI_COUNT) {
+		virq = irq_find_mapping(pcie->irq_domain, pos);
+		if (virq)
+			generic_handle_irq(virq);
+		else
+			pr_err("Unhandled MSI: %d\n", pos);
+	}
+
+	chained_irq_exit(chip, desc);
+}
+
+static struct irq_chip tango_msi_irq_chip = {
+	.name = "MSI",
+	.irq_mask = pci_msi_mask_irq,
+	.irq_unmask = pci_msi_unmask_irq,
+};
+
+static struct msi_domain_info msi_domain_info = {
+	.flags	= MSI_FLAG_USE_DEF_DOM_OPS | MSI_FLAG_USE_DEF_CHIP_OPS,
+	.chip	= &tango_msi_irq_chip,
+};
+
+static void tango_compose_msi_msg(struct irq_data *data, struct msi_msg *msg)
+{
+	struct tango_pcie *pcie = irq_data_get_irq_chip_data(data);
+
+	msg->address_lo = lower_32_bits(pcie->msi_doorbell);
+	msg->address_hi = upper_32_bits(pcie->msi_doorbell);
+	msg->data = data->hwirq;
+}
+
+static int tango_set_affinity(struct irq_data *irq_data,
+		const struct cpumask *mask, bool force)
+{
+	 return -EINVAL;
+}
+
+static struct irq_chip tango_msi_chip = {
+	.name			= "MSI",
+	.irq_compose_msi_msg	= tango_compose_msi_msg,
+	.irq_set_affinity	= tango_set_affinity,
+};
+
+static int tango_irq_domain_alloc(struct irq_domain *domain, unsigned int virq,
+		unsigned int nr_irqs, void *args)
+{
+	struct tango_pcie *pcie = domain->host_data;
+	int pos, err = 0;
+	u32 mask;
+
+	if (nr_irqs != 1) /* When does that happen? */
+		return -EINVAL;
+
+	mutex_lock(&pcie->lock);
+
+	mask = readl_relaxed(pcie->msi_mask);
+	pos = find_first_zero_bit(&mask, MSI_COUNT);
+	if (pos < MSI_COUNT)
+		writel(mask | BIT(pos), pcie->msi_mask);
+	else
+		err = -ENOSPC;
+
+	mutex_unlock(&pcie->lock);
+
+	irq_domain_set_info(domain, virq, pos, &tango_msi_chip,
+			domain->host_data, handle_simple_irq, NULL, NULL);
+
+	return err;
+}
+
+static void tango_irq_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 tango_pcie *pcie = irq_data_get_irq_chip_data(d);
+	int pos = d->hwirq;
+	u32 mask;
+
+	mutex_lock(&pcie->lock);
+
+	mask = readl(pcie->msi_mask);
+	writel(mask & ~BIT(pos), pcie->msi_mask);
+
+	mutex_unlock(&pcie->lock);
+}
+
+static const struct irq_domain_ops msi_domain_ops = {
+	.alloc	= tango_irq_domain_alloc,
+	.free	= tango_irq_domain_free,
+};
+
+static int tango_msi_remove(struct platform_device *pdev)
+{
+	struct tango_pcie *msi = platform_get_drvdata(pdev);
+
+	irq_set_chained_handler(msi->irq, NULL);
+	irq_set_handler_data(msi->irq, NULL);
+	/* irq_set_chained_handler_and_data(msi->irq, NULL, NULL); instead? */
+
+	irq_domain_remove(msi->msi_domain);
+	irq_domain_remove(msi->irq_domain);
+
+	return 0;
+}
+
+static int tango_msi_probe(struct platform_device *pdev, struct tango_pcie *pcie)
+{
+	int virq;
+	struct fwnode_handle *fwnode = of_node_to_fwnode(pdev->dev.of_node);
+	struct irq_domain *msi_dom, *irq_dom;
+
+	mutex_init(&pcie->lock);
+	writel(0, pcie->msi_mask);
+
+	/* Why is fwnode for this call? */
+	irq_dom = irq_domain_add_linear(NULL, MSI_COUNT, &msi_domain_ops, pcie);
+	if (!irq_dom) {
+		pr_err("Failed to create IRQ domain\n");
+		return -ENOMEM;
+	}
+
+	msi_dom = pci_msi_create_irq_domain(fwnode, &msi_domain_info, irq_dom);
+	if (!msi_dom) {
+		pr_err("Failed to create MSI domain\n");
+		irq_domain_remove(irq_dom);
+		return -ENOMEM;
+	}
+
+	virq = platform_get_irq(pdev, 1);
+	if (virq <= 0) {
+		irq_domain_remove(msi_dom);
+		irq_domain_remove(irq_dom);
+		return -ENXIO;
+	}
+
+	pcie->irq_domain = irq_dom;
+	pcie->msi_domain = msi_dom;
+	pcie->irq = virq;
+	irq_set_chained_handler_and_data(virq, tango_msi_isr, pcie);
+
+	return 0;
+}
+
+/*** HOST BRIDGE SUPPORT ***/
+
+static int smp8759_config_read(struct pci_bus *bus,
+		unsigned int devfn, int where, int size, u32 *val)
+{
+	int ret;
+	struct pci_config_window *cfg = bus->sysdata;
+	struct tango_pcie *pcie = dev_get_drvdata(cfg->parent);
+
+	/*
+	 * QUIRK #1
+	 * Reads in configuration space outside devfn 0 return garbage.
+	 */
+	if (devfn != 0) {
+		*val = 0xffffffff; /* ~0 means "nothing here" right? */
+		return PCIBIOS_SUCCESSFUL; /* Should we return error or success? */
+	}
+
+	/*
+	 * QUIRK #2
+	 * The root complex advertizes a fake BAR, which is used to filter
+	 * bus-to-system requests. Hide it from Linux.
+	 */
+	if (where == PCI_BASE_ADDRESS_0 && bus->number == 0) {
+		*val = 0; /* 0 or ~0 to hide the BAR from Linux? */
+		return PCIBIOS_SUCCESSFUL; /* Should we return error or success? */
+	}
+
+	/*
+	 * QUIRK #3
+	 * Unfortunately, config and mem spaces are muxed.
+	 * Linux does not support such a setting, since drivers are free
+	 * to access mem space directly, at any time.
+	 * Therefore, we can only PRAY that config and mem space accesses
+	 * NEVER occur concurrently.
+	 */
+	writel(1, pcie->mux);
+	ret = pci_generic_config_read(bus, devfn, where, size, val);
+	writel(0, pcie->mux);
+
+	return ret;
+}
+
+static int smp8759_config_write(struct pci_bus *bus,
+		unsigned int devfn, int where, int size, u32 val)
+{
+	int ret;
+	struct pci_config_window *cfg = bus->sysdata;
+	struct tango_pcie *pcie = dev_get_drvdata(cfg->parent);
+
+	writel(1, pcie->mux);
+	ret = pci_generic_config_write(bus, devfn, where, size, val);
+	writel(0, pcie->mux);
+
+	return ret;
+}
+
+static struct pci_ecam_ops smp8759_ecam_ops = {
+	.bus_shift	= 20,
+	.pci_ops	= {
+		.map_bus	= pci_ecam_map_bus,
+		.read		= smp8759_config_read,
+		.write		= smp8759_config_write,
+	}
+};
+
+static const struct of_device_id tango_pcie_ids[] = {
+	{ .compatible = "sigma,smp8759-pcie" },
+	{ .compatible = "sigma,rev2-pcie" },
+	{ /* sentinel */ },
+};
+
+static void smp8759_init(struct tango_pcie *pcie, void __iomem *base)
+{
+	pcie->mux		= base + 0x48;
+	pcie->msi_status	= base + 0x80;
+	pcie->msi_mask		= base + 0xa0;
+	pcie->msi_doorbell	= 0xa0000000 + 0x2e07c;
+}
+
+static void rev2_init(struct tango_pcie *pcie, void __iomem *base)
+{
+	void __iomem *misc_irq	= base + 0x40;
+	void __iomem *doorbell	= base + 0x8c;
+
+	pcie->mux		= base + 0x2c;
+	pcie->msi_status	= base + 0x4c;
+	pcie->msi_mask		= base + 0x6c;
+	pcie->msi_doorbell	= 0x80000000;
+
+	writel(lower_32_bits(pcie->msi_doorbell), doorbell + 0);
+	writel(upper_32_bits(pcie->msi_doorbell), doorbell + 4);
+
+	/* Enable legacy PCI interrupts */
+	writel(BIT(15), misc_irq);
+	writel(0xf << 4, misc_irq + 4);
+}
+
+static int tango_pcie_probe(struct platform_device *pdev)
+{
+	int ret;
+	void __iomem *base;
+	struct resource *res;
+	struct tango_pcie *pcie;
+	struct device *dev = &pdev->dev;
+
+	pcie = devm_kzalloc(dev, sizeof(*pcie), GFP_KERNEL);
+	if (!pcie)
+		return -ENOMEM;
+
+	platform_set_drvdata(pdev, pcie);
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
+	base = devm_ioremap_resource(&pdev->dev, res);
+	if (IS_ERR(base))
+		return PTR_ERR(base);
+
+	if (of_device_is_compatible(dev->of_node, "sigma,smp8759-pcie"))
+		smp8759_init(pcie, base);
+
+	if (of_device_is_compatible(dev->of_node, "sigma,rev2-pcie"))
+		rev2_init(pcie, base);
+
+	ret = tango_msi_probe(pdev, pcie);
+	if (ret)
+		return ret;
+
+	return pci_host_common_probe(pdev, &smp8759_ecam_ops);
+}
+
+static int tango_pcie_remove(struct platform_device *pdev)
+{
+	return tango_msi_remove(pdev);
+}
+
+static struct platform_driver tango_pcie_driver = {
+	.probe	= tango_pcie_probe,
+	.remove	= tango_pcie_remove,
+	.driver	= {
+		.name = KBUILD_MODNAME,
+		.of_match_table = tango_pcie_ids,
+	},
+};
+
+/*
+ * This should probably be module_platform_driver ?
+ */
+builtin_platform_driver(tango_pcie_driver);
+
+#define VENDOR_SIGMA	0x1105
+
+/*
+ * QUIRK #4
+ * The root complex advertizes the wrong device class.
+ * Header Type 1 is for PCI-to-PCI bridges.
+ */
+static void tango_fixup_class(struct pci_dev *dev)
+{
+	dev->class = PCI_CLASS_BRIDGE_PCI << 8;
+}
+DECLARE_PCI_FIXUP_EARLY(VENDOR_SIGMA, PCI_ANY_ID, tango_fixup_class);
+
+/*
+ * QUIRK #5
+ * Only transfers within the root complex BAR are forwarded to the host.
+ * By default, the DMA framework expects that
+ * PCI address 0x8000_0000 maps to system address 0x8000_0000
+ * which is where DRAM0 is mapped.
+ */
+static void tango_fixup_bar(struct pci_dev *dev)
+{
+        pci_write_config_dword(dev, PCI_BASE_ADDRESS_0, 0x80000000);
+}
+DECLARE_PCI_FIXUP_FINAL(VENDOR_SIGMA, PCI_ANY_ID, tango_fixup_bar);
-- 
2.11.0

[toc] | [next] | [standalone]

#1607540

On 23/03/17 13:05, Mason wrote:
> I think this version is ready for review.
> It has all the required bits and pieces.
> I still have a few questions, embedded as comments in the code.
> (Missing are ancillary changes to Kconfig, Makefile)

May I suggest that if you think that a patch is ready for review, it
should really contain all the bits that make it an actual patch? That
would include an actual commit log and all what is required to actually
compile it. Not to mention a SoB.

We rely (at least I certainly do) on things like the kbuild robot
picking up stuff from the list and giving it a go. Also, it makes it a
much more efficient use of the reviewer time not to review the same
thing twice...

That being said:

> ---
>  drivers/pci/host/pcie-tango.c | 350 ++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 350 insertions(+)
>  create mode 100644 drivers/pci/host/pcie-tango.c
> 
> diff --git a/drivers/pci/host/pcie-tango.c b/drivers/pci/host/pcie-tango.c
> new file mode 100644
> index 000000000000..b2e6448aed2d
> --- /dev/null
> +++ b/drivers/pci/host/pcie-tango.c
> @@ -0,0 +1,350 @@
> +#include <linux/irqchip/chained_irq.h>
> +#include <linux/irqdomain.h>
> +#include <linux/pci-ecam.h>
> +#include <linux/msi.h>
> +
> +#define MSI_COUNT 32

Is this something that is hardcoded? Unlikely to ever change?

> +
> +struct tango_pcie {
> +	void __iomem *mux;
> +	void __iomem *msi_status;
> +	void __iomem *msi_mask;
> +	phys_addr_t msi_doorbell;
> +	struct mutex lock; /* lock for updating msi_mask */
> +	struct irq_domain *irq_domain;
> +	struct irq_domain *msi_domain;
> +	int irq;
> +};
> +
> +/*** MSI CONTROLLER SUPPORT ***/
> +
> +static void tango_msi_isr(struct irq_desc *desc)
> +{
> +	struct irq_chip *chip = irq_desc_get_chip(desc);
> +	struct tango_pcie *pcie;
> +	unsigned long status, virq;
> +	int pos;
> +
> +	chained_irq_enter(chip, desc);
> +	pcie = irq_desc_get_handler_data(desc);
> +
> +	status = readl_relaxed(pcie->msi_status);

Please use types that unambiguously match that of the MMIO accessor (u32
in this case). On a 64bit system, unsigned long is likely to be 64bit.
You can assign it to an unsigned long before calling the
for_each_set_bit operator.

> +	writel_relaxed(status, pcie->msi_status); /* clear IRQs */

Why isn't this your irq_ack method instead of open-coding it?

> +
> +	for_each_set_bit(pos, &status, MSI_COUNT) {
> +		virq = irq_find_mapping(pcie->irq_domain, pos);
> +		if (virq)
> +			generic_handle_irq(virq);
> +		else
> +			pr_err("Unhandled MSI: %d\n", pos);

Please rate-limit this.

> +	}
> +
> +	chained_irq_exit(chip, desc);
> +}
> +
> +static struct irq_chip tango_msi_irq_chip = {
> +	.name = "MSI",
> +	.irq_mask = pci_msi_mask_irq,
> +	.irq_unmask = pci_msi_unmask_irq,
> +};
> +
> +static struct msi_domain_info msi_domain_info = {
> +	.flags	= MSI_FLAG_USE_DEF_DOM_OPS | MSI_FLAG_USE_DEF_CHIP_OPS,

No support for MSI-X? Why?

> +	.chip	= &tango_msi_irq_chip,
> +};
> +
> +static void tango_compose_msi_msg(struct irq_data *data, struct msi_msg *msg)
> +{
> +	struct tango_pcie *pcie = irq_data_get_irq_chip_data(data);
> +
> +	msg->address_lo = lower_32_bits(pcie->msi_doorbell);
> +	msg->address_hi = upper_32_bits(pcie->msi_doorbell);
> +	msg->data = data->hwirq;
> +}
> +
> +static int tango_set_affinity(struct irq_data *irq_data,
> +		const struct cpumask *mask, bool force)
> +{
> +	 return -EINVAL;
> +}
> +
> +static struct irq_chip tango_msi_chip = {
> +	.name			= "MSI",
> +	.irq_compose_msi_msg	= tango_compose_msi_msg,
> +	.irq_set_affinity	= tango_set_affinity,
> +};
> +
> +static int tango_irq_domain_alloc(struct irq_domain *domain, unsigned int virq,
> +		unsigned int nr_irqs, void *args)
> +{
> +	struct tango_pcie *pcie = domain->host_data;
> +	int pos, err = 0;
> +	u32 mask;
> +
> +	if (nr_irqs != 1) /* When does that happen? */
> +		return -EINVAL;

Only if the end-point wants to use Multi-MSI. You don't advertise
support for it, so it should never happen.

> +
> +	mutex_lock(&pcie->lock);
> +
> +	mask = readl_relaxed(pcie->msi_mask);

Do you really need to read this from the HW each time you allocate an
interrupt? That feels pretty crazy. You're much better off having an
in-memory bitmap that will make things more efficient, and avoid the
following bug...

> +	pos = find_first_zero_bit(&mask, MSI_COUNT);

... where using a u32 as a bitmap is a very bad idea (because not the
whole world is a 32bit, little endian platform).

> +	if (pos < MSI_COUNT)
> +		writel(mask | BIT(pos), pcie->msi_mask);

And it would make a lot more sense to move this write (which should be
relaxed) to irq_unmask. Also, calling msi_mask for something that is an
enable register is a bit counter intuitive.

> +	else
> +		err = -ENOSPC;
> +
> +	mutex_unlock(&pcie->lock);
> +
> +	irq_domain_set_info(domain, virq, pos, &tango_msi_chip,
> +			domain->host_data, handle_simple_irq, NULL, NULL);

And here, you're polluting the domain even if you failed to allocate the
interrupt.

> +
> +	return err;
> +}
> +
> +static void tango_irq_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 tango_pcie *pcie = irq_data_get_irq_chip_data(d);
> +	int pos = d->hwirq;
> +	u32 mask;
> +
> +	mutex_lock(&pcie->lock);
> +
> +	mask = readl(pcie->msi_mask);
> +	writel(mask & ~BIT(pos), pcie->msi_mask);

Same as above, please move this to the irq_unmask method.

> +
> +	mutex_unlock(&pcie->lock);
> +}
> +
> +static const struct irq_domain_ops msi_domain_ops = {
> +	.alloc	= tango_irq_domain_alloc,
> +	.free	= tango_irq_domain_free,
> +};
> +
> +static int tango_msi_remove(struct platform_device *pdev)
> +{
> +	struct tango_pcie *msi = platform_get_drvdata(pdev);
> +
> +	irq_set_chained_handler(msi->irq, NULL);
> +	irq_set_handler_data(msi->irq, NULL);
> +	/* irq_set_chained_handler_and_data(msi->irq, NULL, NULL); instead? */
> +
> +	irq_domain_remove(msi->msi_domain);
> +	irq_domain_remove(msi->irq_domain);
> +
> +	return 0;
> +}
> +
> +static int tango_msi_probe(struct platform_device *pdev, struct tango_pcie *pcie)
> +{
> +	int virq;
> +	struct fwnode_handle *fwnode = of_node_to_fwnode(pdev->dev.of_node);
> +	struct irq_domain *msi_dom, *irq_dom;
> +
> +	mutex_init(&pcie->lock);
> +	writel(0, pcie->msi_mask);
> +
> +	/* Why is fwnode for this call? */
> +	irq_dom = irq_domain_add_linear(NULL, MSI_COUNT, &msi_domain_ops, pcie);

Use irq_domain_create_linear, pass the same fwnode.

> +	if (!irq_dom) {
> +		pr_err("Failed to create IRQ domain\n");
> +		return -ENOMEM;
> +	}
> +
> +	msi_dom = pci_msi_create_irq_domain(fwnode, &msi_domain_info, irq_dom);
> +	if (!msi_dom) {
> +		pr_err("Failed to create MSI domain\n");
> +		irq_domain_remove(irq_dom);
> +		return -ENOMEM;
> +	}
> +
> +	virq = platform_get_irq(pdev, 1);

In the absence of a documented binding, it is hard to know if you're
doing the right thing.

> +	if (virq <= 0) {
> +		irq_domain_remove(msi_dom);
> +		irq_domain_remove(irq_dom);
> +		return -ENXIO;

Maybe add a message indicating what failed?

> +	}
> +
> +	pcie->irq_domain = irq_dom;
> +	pcie->msi_domain = msi_dom;
> +	pcie->irq = virq;
> +	irq_set_chained_handler_and_data(virq, tango_msi_isr, pcie);
> +
> +	return 0;
> +}
> +
> +/*** HOST BRIDGE SUPPORT ***/

[...]

I don't know much about PCIe itself, hence stopping here.

I'd like to see the MSI code as a separate patch, because it is pretty
much standalone. And please write a DT binding document for the whole
thing, because I end-up second guessing what you're trying to do...

Thanks,

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

[toc] | [prev] | [next] | [standalone]

#1607722

On 23/03/2017 15:22, Marc Zyngier wrote:

> On 23/03/17 13:05, Mason wrote:
> 
>> +#define MSI_COUNT 32
> 
> Is this something that is hardcoded? Unlikely to ever change?

The host bridge actually supports 256 MSIs.

IIUC, what you suggested on IRC is that I support 256 in the driver,
and only read the status for *enabled* MSIs.

Pseudo-code:

for every 32-bit blob in the enabled bitmap
  if the value is non-zero
    lookup the corresponding status reg

Problem is that a BITMAP is unsigned long (as you point out below).
So I'm not sure how to iterate 32-bits at a time over the BITMAP.




>> +static void tango_msi_isr(struct irq_desc *desc)
>> +{
>> +	struct irq_chip *chip = irq_desc_get_chip(desc);
>> +	struct tango_pcie *pcie;
>> +	unsigned long status, virq;
>> +	int pos;
>> +
>> +	chained_irq_enter(chip, desc);
>> +	pcie = irq_desc_get_handler_data(desc);
>> +
>> +	status = readl_relaxed(pcie->msi_status);
> 
> Please use types that unambiguously match that of the MMIO accessor (u32
> in this case). On a 64bit system, unsigned long is likely to be 64bit.
> You can assign it to an unsigned long before calling the
> for_each_set_bit operator.

OK. I'm aware that unsigned long is 64 bits on sane 64b platforms,
but since extending u32 to u64 would pad with zeros, I didn't expect
this to be an issue. I will change the code. Note: I copied the
code from the Altera driver.

>> +	writel_relaxed(status, pcie->msi_status); /* clear IRQs */
> 
> Why isn't this your irq_ack method instead of open-coding it?

I based my driver on the Altera driver, and I did it like
I thought they did. I will try fixing my code.


>> +	for_each_set_bit(pos, &status, MSI_COUNT) {
>> +		virq = irq_find_mapping(pcie->irq_domain, pos);
>> +		if (virq)
>> +			generic_handle_irq(virq);
>> +		else
>> +			pr_err("Unhandled MSI: %d\n", pos);
> 
> Please rate-limit this.

I'll use pr_err_ratelimited


>> +static struct msi_domain_info msi_domain_info = {
>> +	.flags	= MSI_FLAG_USE_DEF_DOM_OPS | MSI_FLAG_USE_DEF_CHIP_OPS,
> 
> No support for MSI-X? Why?

Good question.
https://en.wikipedia.org/wiki/Message_Signaled_Interrupts#MSI-X
My controller supports a single doorbell, and only 256 MSIs.
I thought that meant it didn't support MSI-X.


>> +static int tango_irq_domain_alloc(struct irq_domain *domain, unsigned int virq,
>> +		unsigned int nr_irqs, void *args)
>> +{
>> +	struct tango_pcie *pcie = domain->host_data;
>> +	int pos, err = 0;
>> +	u32 mask;
>> +
>> +	if (nr_irqs != 1) /* When does that happen? */
>> +		return -EINVAL;
> 
> Only if the end-point wants to use Multi-MSI. You don't advertise
> support for it, so it should never happen.

Should I keep the test or remove it?


>> +	mutex_lock(&pcie->lock);
>> +
>> +	mask = readl_relaxed(pcie->msi_mask);
> 
> Do you really need to read this from the HW each time you allocate an
> interrupt? That feels pretty crazy. You're much better off having an
> in-memory bitmap that will make things more efficient, and avoid the
> following bug...
> 
>> +	pos = find_first_zero_bit(&mask, MSI_COUNT);
> 
> ... where using a u32 as a bitmap is a very bad idea (because not the
> whole world is a 32bit, little endian platform).

I understand your point. This ties in to the ISR discussion.


>> +	if (pos < MSI_COUNT)
>> +		writel(mask | BIT(pos), pcie->msi_mask);
> 
> And it would make a lot more sense to move this write (which should be
> relaxed) to irq_unmask. Also, calling msi_mask for something that is an
> enable register is a bit counter intuitive.

I don't have as much experience as you.
I just used the names in the HW documentation.
I think it is the "mask" (as in bitmap) of enabled MSIs.
I will change "mask" to "enable".

Are you saying I should not use pci_msi_mask_irq and pci_msi_unmask_irq,
but register custom implementations? I should still call these in my
custom functions, right?


>> +	else
>> +		err = -ENOSPC;
>> +
>> +	mutex_unlock(&pcie->lock);
>> +
>> +	irq_domain_set_info(domain, virq, pos, &tango_msi_chip,
>> +			domain->host_data, handle_simple_irq, NULL, NULL);
> 
> And here, you're polluting the domain even if you failed to allocate the
> interrupt.

This bug is 100% mine. Will fix.

>> +
>> +	return err;
>> +}
>> +
>> +static void tango_irq_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 tango_pcie *pcie = irq_data_get_irq_chip_data(d);
>> +	int pos = d->hwirq;
>> +	u32 mask;
>> +
>> +	mutex_lock(&pcie->lock);
>> +
>> +	mask = readl(pcie->msi_mask);
>> +	writel(mask & ~BIT(pos), pcie->msi_mask);
> 
> Same as above, please move this to the irq_unmask method.

This one should be irq_mask, no?

Even If I move the MMIO write, it should be done under lock,
I think. But I don't know in what context irq_unmask will
be called.
You said: not mutex, spinlock.


>> +static int tango_msi_remove(struct platform_device *pdev)
>> +{
>> +	struct tango_pcie *msi = platform_get_drvdata(pdev);
>> +
>> +	irq_set_chained_handler(msi->irq, NULL);
>> +	irq_set_handler_data(msi->irq, NULL);
>> +	/* irq_set_chained_handler_and_data(msi->irq, NULL, NULL); instead? */

Can I call irq_set_chained_handler_and_data(msi->irq, NULL, NULL);
instead of the two calls?


>> +	mutex_init(&pcie->lock);
>> +	writel(0, pcie->msi_mask);
>> +
>> +	/* Why is fwnode for this call? */
>> +	irq_dom = irq_domain_add_linear(NULL, MSI_COUNT, &msi_domain_ops, pcie);
> 
> Use irq_domain_create_linear, pass the same fwnode.

Will change that.


>> +	if (!irq_dom) {
>> +		pr_err("Failed to create IRQ domain\n");
>> +		return -ENOMEM;
>> +	}
>> +
>> +	msi_dom = pci_msi_create_irq_domain(fwnode, &msi_domain_info, irq_dom);
>> +	if (!msi_dom) {
>> +		pr_err("Failed to create MSI domain\n");
>> +		irq_domain_remove(irq_dom);
>> +		return -ENOMEM;
>> +	}
>> +
>> +	virq = platform_get_irq(pdev, 1);
> 
> In the absence of a documented binding, it is hard to know if you're
> doing the right thing.

		pcie@50000000 {
			compatible = "sigma,smp8759-pcie";
			reg = <0x50000000 SZ_64M>, <0x2e000 0x100>;
			device_type = "pci";
			bus-range = <0 63>;
			#size-cells = <2>;
			#address-cells = <3>;
			#interrupt-cells = <1>;
			ranges = <0x02000000 0x0 0x04000000  0x54000000  0x0 SZ_192M>;
			msi-controller;
			/* 54 for misc interrupts, 55 for MSI */
			interrupts = <54 IRQ_TYPE_LEVEL_HIGH>, <55 IRQ_TYPE_LEVEL_HIGH>;
		};

Note: I don't have an "interrupt-map" prop because rev1 doesn't support
legacy PCI interrupts (INTx). But I see the PCI framework wrongly mapping
intA to my system's interrupt #1, presumably because I am lacking an
interrupt-map?

Also I find the MSI interrupt number to be high:

# cat /proc/interrupts 
           CPU0       CPU1       
 19:      21171       1074     GIC-0  29 Edge      twd
 20:        116          0      irq0   1 Level     serial
 26:          7          0       MSI   0 Edge      aerdrv
 28:       3263          0       MSI 524288 Edge      xhci_hcd

524288 is 0x80000. Was this offset chosen by the intc core?
Or by my (lack of) DT?

>> +	if (virq <= 0) {
>> +		irq_domain_remove(msi_dom);
>> +		irq_domain_remove(irq_dom);
>> +		return -ENXIO;
> 
> Maybe add a message indicating what failed?

Will do.

Thanks again for the thorough review.

Regards.

[toc] | [prev] | [next] | [standalone]

#1608019

On 23/03/2017 18:03, Mason wrote:

> The host bridge actually supports 256 MSIs.
> 
> IIUC, what you suggested on IRC is that I support 256 in the driver,
> and only read the status for *enabled* MSIs.
> 
> Pseudo-code:
> 
> for every 32-bit blob in the enabled bitmap
>   if the value is non-zero
>     lookup the corresponding status reg
> 
> Problem is that a BITMAP is unsigned long (as you point out below).
> So I'm not sure how to iterate 32-bits at a time over the BITMAP.

Something along these lines:

	DECLARE_BITMAP(enabled, 256);

	unsigned int pos = 0;

	while ((pos = find_next_bit(enabled, 256, pos)) < 256) {
		int offset = (pos / 32) * 4;
		u32 status = readl_relaxed(status + offset);
		/* Handle each pos set in status */
		pos = round_up(pos, 32);
	}

You mentioned a bug in my code (due to the platform endianness)
when passing the result of readl_relaxed to the bitops routine...
How is one supposed to iterate over status?

I'm not yet seeing this endianness issue, since (status & BIT(i))
provides the status of MSI_i, irrespective of endianness.

Although I see that arch/arm/include/asm/bitops.h declares
BE and LE variants... I'm confused.

Regards.

[toc] | [prev] | [next] | [standalone]

#1608841

On 23/03/17 23:40, Mason wrote:
> On 23/03/2017 18:03, Mason wrote:
> 
>> The host bridge actually supports 256 MSIs.
>>
>> IIUC, what you suggested on IRC is that I support 256 in the driver,
>> and only read the status for *enabled* MSIs.
>>
>> Pseudo-code:
>>
>> for every 32-bit blob in the enabled bitmap
>>   if the value is non-zero
>>     lookup the corresponding status reg
>>
>> Problem is that a BITMAP is unsigned long (as you point out below).
>> So I'm not sure how to iterate 32-bits at a time over the BITMAP.
> 
> Something along these lines:
> 
> 	DECLARE_BITMAP(enabled, 256);
> 
> 	unsigned int pos = 0;
> 
> 	while ((pos = find_next_bit(enabled, 256, pos)) < 256) {
> 		int offset = (pos / 32) * 4;
> 		u32 status = readl_relaxed(status + offset);
> 		/* Handle each pos set in status */
> 		pos = round_up(pos, 32);
> 	}

Something along those lines, yes.

> You mentioned a bug in my code (due to the platform endianness)
> when passing the result of readl_relaxed to the bitops routine...
> How is one supposed to iterate over status?

You cannot directly use a pointer to a u32 in any of the bitmap
operations. You need to copy the value to an unsigned long, and apply
the bitmap op on that.

> I'm not yet seeing this endianness issue, since (status & BIT(i))
> provides the status of MSI_i, irrespective of endianness.

We discussed this over IRC, but I was referring to the above case and
64bit BE platforms, which would not do what you expect.

> Although I see that arch/arm/include/asm/bitops.h declares
> BE and LE variants... I'm confused.

Nothing in this code should be ARM specific. The kernel gives you the
tools to write (mostly) architecture, endianness and word size agnostic
code.

Thanks,

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

[toc] | [prev] | [next] | [standalone]

#1609893

On 24/03/2017 19:22, Marc Zyngier wrote:

> You cannot directly use a pointer to a u32 in any of the bitmap
> operations. You need to copy the value to an unsigned long, and
> apply the bitmap op on that.

On my platform, find_first_zero_bit() resolves to

  int _find_first_zero_bit_le(const void * p, unsigned size);

If the underlying implementation actually expects an unsigned long
pointer, should the function prototype be changed?

Regards.

[toc] | [prev] | [next] | [standalone]

#1609898

On Mon, 27 Mar 2017, Mason wrote:
> On 24/03/2017 19:22, Marc Zyngier wrote:
> 
> > You cannot directly use a pointer to a u32 in any of the bitmap
> > operations. You need to copy the value to an unsigned long, and
> > apply the bitmap op on that.
> 
> On my platform, find_first_zero_bit() resolves to
> 
>   int _find_first_zero_bit_le(const void * p, unsigned size);
> 
> If the underlying implementation actually expects an unsigned long
> pointer, should the function prototype be changed?

Errm? Why are you worrying about the underlying implementations?

find_first_zero_bit() is what you are supposed to use in your code. And
that explicitely takes a unsigned long pointer.

Thanks,

	tglx

[toc] | [prev] | [next] | [standalone]

#1609938

On 27/03/2017 16:46, Thomas Gleixner wrote:

> On Mon, 27 Mar 2017, Mason wrote:
>
>> On 24/03/2017 19:22, Marc Zyngier wrote:
>>
>>> You cannot directly use a pointer to a u32 in any of the bitmap
>>> operations. You need to copy the value to an unsigned long, and
>>> apply the bitmap op on that.
>>
>> On my platform, find_first_zero_bit() resolves to
>>
>>   int _find_first_zero_bit_le(const void * p, unsigned size);
>>
>> If the underlying implementation actually expects an unsigned long
>> pointer, should the function prototype be changed?
> 
> Errm? Why are you worrying about the underlying implementations?
> 
> find_first_zero_bit() is what you are supposed to use in your code. And
> that explicitely takes a unsigned long pointer.

I don't think so.

If the prototype for find_first_zero_bit() specified the first
argument as an unsigned long pointer, then the compiler would
have rejected my code like this:

  CC      drivers/pci/host/pcie-tango.o
In file included from ./include/linux/bitops.h:36:0,
                 from ./include/linux/kernel.h:10,
                 from ./include/linux/list.h:8,
                 from ./include/linux/smp.h:11,
                 from ./include/linux/irq.h:12,
                 from ./include/linux/irqchip/chained_irq.h:21,
                 from drivers/pci/host/pcie-tango.c:1:
drivers/pci/host/pcie-tango.c: In function 'tango_irq_domain_alloc':
drivers/pci/host/pcie-tango.c:122:28: error: passing argument 1 of '_find_first_zero_bit_le' from incompatible pointer type [-Werror=incompatible-pointer-types]
  pos = find_first_zero_bit(&mask, 32);
                            ^
./arch/arm/include/asm/bitops.h:199:59: note: in definition of macro 'find_first_zero_bit'
 #define find_first_zero_bit(p,sz) _find_first_zero_bit_le(p,sz)
                                                           ^
./arch/arm/include/asm/bitops.h:162:12: note: expected 'const long unsigned int *' but argument is of type 'u32 * {aka unsigned int *}'
 extern int _find_first_zero_bit_le(const unsigned long * p, unsigned size);
            ^
cc1: some warnings being treated as errors
make[1]: *** [drivers/pci/host/pcie-tango.o] Error 1
make: *** [drivers/pci/host/pcie-tango.o] Error 2


But, in fact, the compiler remained silent, specifically because
the situation on my platform is:

  #define find_first_zero_bit(p,sz)	_find_first_zero_bit_le(p,sz)
  int _find_first_zero_bit_le(const void * p, unsigned size);


So I asked if the prototype could/should be changed, to have the
compiler catch the error as early as possible.

Regards.

[toc] | [prev] | [next] | [standalone]

#1608867

On 23/03/17 17:03, Mason wrote:
> On 23/03/2017 15:22, Marc Zyngier wrote:
> 
>> On 23/03/17 13:05, Mason wrote:
>>
>>> +#define MSI_COUNT 32
>>
>> Is this something that is hardcoded? Unlikely to ever change?
> 
> The host bridge actually supports 256 MSIs.
> 
> IIUC, what you suggested on IRC is that I support 256 in the driver,
> and only read the status for *enabled* MSIs.
> 
> Pseudo-code:
> 
> for every 32-bit blob in the enabled bitmap
>   if the value is non-zero
>     lookup the corresponding status reg
> 
> Problem is that a BITMAP is unsigned long (as you point out below).
> So I'm not sure how to iterate 32-bits at a time over the BITMAP.

See my reply in a previous email.

>>> +static void tango_msi_isr(struct irq_desc *desc)
>>> +{
>>> +	struct irq_chip *chip = irq_desc_get_chip(desc);
>>> +	struct tango_pcie *pcie;
>>> +	unsigned long status, virq;
>>> +	int pos;
>>> +
>>> +	chained_irq_enter(chip, desc);
>>> +	pcie = irq_desc_get_handler_data(desc);
>>> +
>>> +	status = readl_relaxed(pcie->msi_status);
>>
>> Please use types that unambiguously match that of the MMIO accessor (u32
>> in this case). On a 64bit system, unsigned long is likely to be 64bit.
>> You can assign it to an unsigned long before calling the
>> for_each_set_bit operator.
> 
> OK. I'm aware that unsigned long is 64 bits on sane 64b platforms,
> but since extending u32 to u64 would pad with zeros, I didn't expect
> this to be an issue. I will change the code. Note: I copied the
> code from the Altera driver.

This is an issue when your system is 64bit BE, something that is not
that uncommon.

> 
>>> +	writel_relaxed(status, pcie->msi_status); /* clear IRQs */
>>
>> Why isn't this your irq_ack method instead of open-coding it?
> 
> I based my driver on the Altera driver, and I did it like
> I thought they did. I will try fixing my code.

Doesn't make it right, unfortunately. I wish you would try to understand
the API first instead of copy-pasting things (including potential bugs).

> 
>>> +	for_each_set_bit(pos, &status, MSI_COUNT) {
>>> +		virq = irq_find_mapping(pcie->irq_domain, pos);
>>> +		if (virq)
>>> +			generic_handle_irq(virq);
>>> +		else
>>> +			pr_err("Unhandled MSI: %d\n", pos);
>>
>> Please rate-limit this.
> 
> I'll use pr_err_ratelimited
> 
> 
>>> +static struct msi_domain_info msi_domain_info = {
>>> +	.flags	= MSI_FLAG_USE_DEF_DOM_OPS | MSI_FLAG_USE_DEF_CHIP_OPS,
>>
>> No support for MSI-X? Why?
> 
> Good question.
> https://en.wikipedia.org/wiki/Message_Signaled_Interrupts#MSI-X
> My controller supports a single doorbell, and only 256 MSIs.
> I thought that meant it didn't support MSI-X.

The "single doorbell" requirement is on the end-point, not on the
controller. Multi-MSI only has a single register for all possible
interrupts, while MSI-X allows one doorbell address per interrupt. In
your case, all interrupts will have the same doorbell address, which is
perfectly fine.

> 
> 
>>> +static int tango_irq_domain_alloc(struct irq_domain *domain, unsigned int virq,
>>> +		unsigned int nr_irqs, void *args)
>>> +{
>>> +	struct tango_pcie *pcie = domain->host_data;
>>> +	int pos, err = 0;
>>> +	u32 mask;
>>> +
>>> +	if (nr_irqs != 1) /* When does that happen? */
>>> +		return -EINVAL;
>>
>> Only if the end-point wants to use Multi-MSI. You don't advertise
>> support for it, so it should never happen.
> 
> Should I keep the test or remove it?

Up to you. Some people warn loudly, some other ignore it, you've chosen
a middle ground.

> 
> 
>>> +	mutex_lock(&pcie->lock);
>>> +
>>> +	mask = readl_relaxed(pcie->msi_mask);
>>
>> Do you really need to read this from the HW each time you allocate an
>> interrupt? That feels pretty crazy. You're much better off having an
>> in-memory bitmap that will make things more efficient, and avoid the
>> following bug...
>>
>>> +	pos = find_first_zero_bit(&mask, MSI_COUNT);
>>
>> ... where using a u32 as a bitmap is a very bad idea (because not the
>> whole world is a 32bit, little endian platform).
> 
> I understand your point. This ties in to the ISR discussion.
> 
> 
>>> +	if (pos < MSI_COUNT)
>>> +		writel(mask | BIT(pos), pcie->msi_mask);
>>
>> And it would make a lot more sense to move this write (which should be
>> relaxed) to irq_unmask. Also, calling msi_mask for something that is an
>> enable register is a bit counter intuitive.
> 
> I don't have as much experience as you.
> I just used the names in the HW documentation.
> I think it is the "mask" (as in bitmap) of enabled MSIs.
> I will change "mask" to "enable".
> 
> Are you saying I should not use pci_msi_mask_irq and pci_msi_unmask_irq,
> but register custom implementations? I should still call these in my
> custom functions, right?

You can call both in your own mask/unmask methods. They serve different
purpose (one is at the endpoint level, the other is at the MSI
controller level).

> 
> 
>>> +	else
>>> +		err = -ENOSPC;
>>> +
>>> +	mutex_unlock(&pcie->lock);
>>> +
>>> +	irq_domain_set_info(domain, virq, pos, &tango_msi_chip,
>>> +			domain->host_data, handle_simple_irq, NULL, NULL);
>>
>> And here, you're polluting the domain even if you failed to allocate the
>> interrupt.
> 
> This bug is 100% mine. Will fix.

Erm. In this file, all bugs are yours! :-)

> 
>>> +
>>> +	return err;
>>> +}
>>> +
>>> +static void tango_irq_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 tango_pcie *pcie = irq_data_get_irq_chip_data(d);
>>> +	int pos = d->hwirq;
>>> +	u32 mask;
>>> +
>>> +	mutex_lock(&pcie->lock);
>>> +
>>> +	mask = readl(pcie->msi_mask);
>>> +	writel(mask & ~BIT(pos), pcie->msi_mask);
>>
>> Same as above, please move this to the irq_unmask method.
> 
> This one should be irq_mask, no?

Yes.

> 
> Even If I move the MMIO write, it should be done under lock,
> I think. But I don't know in what context irq_unmask will
> be called.
> You said: not mutex, spinlock.

It can be called from interrupt context, so it cannot be a mutex.

> 
> 
>>> +static int tango_msi_remove(struct platform_device *pdev)
>>> +{
>>> +	struct tango_pcie *msi = platform_get_drvdata(pdev);
>>> +
>>> +	irq_set_chained_handler(msi->irq, NULL);
>>> +	irq_set_handler_data(msi->irq, NULL);
>>> +	/* irq_set_chained_handler_and_data(msi->irq, NULL, NULL); instead? */
> 
> Can I call irq_set_chained_handler_and_data(msi->irq, NULL, NULL);
> instead of the two calls?

Probably. Reading the code should tell you.

> 
>>> +	mutex_init(&pcie->lock);
>>> +	writel(0, pcie->msi_mask);
>>> +
>>> +	/* Why is fwnode for this call? */
>>> +	irq_dom = irq_domain_add_linear(NULL, MSI_COUNT, &msi_domain_ops, pcie);
>>
>> Use irq_domain_create_linear, pass the same fwnode.
> 
> Will change that.
> 
> 
>>> +	if (!irq_dom) {
>>> +		pr_err("Failed to create IRQ domain\n");
>>> +		return -ENOMEM;
>>> +	}
>>> +
>>> +	msi_dom = pci_msi_create_irq_domain(fwnode, &msi_domain_info, irq_dom);
>>> +	if (!msi_dom) {
>>> +		pr_err("Failed to create MSI domain\n");
>>> +		irq_domain_remove(irq_dom);
>>> +		return -ENOMEM;
>>> +	}
>>> +
>>> +	virq = platform_get_irq(pdev, 1);
>>
>> In the absence of a documented binding, it is hard to know if you're
>> doing the right thing.
> 
> 		pcie@50000000 {
> 			compatible = "sigma,smp8759-pcie";
> 			reg = <0x50000000 SZ_64M>, <0x2e000 0x100>;
> 			device_type = "pci";
> 			bus-range = <0 63>;
> 			#size-cells = <2>;
> 			#address-cells = <3>;
> 			#interrupt-cells = <1>;
> 			ranges = <0x02000000 0x0 0x04000000  0x54000000  0x0 SZ_192M>;
> 			msi-controller;
> 			/* 54 for misc interrupts, 55 for MSI */
> 			interrupts = <54 IRQ_TYPE_LEVEL_HIGH>, <55 IRQ_TYPE_LEVEL_HIGH>;
> 		};

This is not a binding. This is an example from your DT. Look at
Documentation/devicetree/bindings/pci/ for examples of the required
documentation.

> 
> Note: I don't have an "interrupt-map" prop because rev1 doesn't support
> legacy PCI interrupts (INTx). But I see the PCI framework wrongly mapping
> intA to my system's interrupt #1, presumably because I am lacking an
> interrupt-map?

Probably. I don't think it is legal not to have an interrupt-map.

> 
> Also I find the MSI interrupt number to be high:
> 
> # cat /proc/interrupts 
>            CPU0       CPU1       
>  19:      21171       1074     GIC-0  29 Edge      twd
>  20:        116          0      irq0   1 Level     serial
>  26:          7          0       MSI   0 Edge      aerdrv
>  28:       3263          0       MSI 524288 Edge      xhci_hcd
> 
> 524288 is 0x80000. Was this offset chosen by the intc core?

By the PCI/MSI layer. See pci_msi_domain_calc_hwirq().

Thanks,

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

[toc] | [prev] | [next] | [standalone]

#1609962

On 24/03/2017 19:47, Marc Zyngier wrote:

> On 23/03/17 17:03, Mason wrote:
>
>> On 23/03/2017 15:22, Marc Zyngier wrote:
>>
>>> On 23/03/17 13:05, Mason wrote:
>>>
>>>> +	writel_relaxed(status, pcie->msi_status); /* clear IRQs */
>>>
>>> Why isn't this your irq_ack method instead of open-coding it?
>>
>> I based my driver on the Altera driver, and I did it like
>> I thought they did. I will try fixing my code.
> 
> Doesn't make it right, unfortunately. I wish you would try to understand
> the API first instead of copy-pasting things (including potential bugs).

So far, I have not been able to get the irqchip framework to
call the irq_ack functions I registered.

Should I pass a different handler than handle_simple_irq
to irq_domain_set_info?

	irq_domain_set_info(domain, virq, pos, &tango_msi_chip,
		domain->host_data, handle_simple_irq, NULL, NULL);


When an MSI packet arrives at the MSI doorbell address, the controller
reads the packet's data; this is the MSI number "num". It sets bit "num"
to 1 in the status regs, and raises IRQ line 55 on the system intc.
The IRQ signal remains high, until software clears it by writing 1
in bit "num" of the status regs.

Is this an edge interrupt or a level interrupt?

I was told if the interrupt request is triggered by an event, then
it is an edge interrupt. The reception of an MSI packet is an event.
But the IRQ remains high, so this feels like a level high.
I'm hopelessly confused :-(


>>>> +	mutex_lock(&pcie->lock);
>>>> +
>>>> +	mask = readl_relaxed(pcie->msi_mask);
>>>
>>> Do you really need to read this from the HW each time you allocate an
>>> interrupt? That feels pretty crazy. You're much better off having an
>>> in-memory bitmap that will make things more efficient [...]

I have one remaining issue with bitmaps.

My HW regs are 32b. How do I grab e.g. bits 96-127?
All I can think of is
  u32 val = ((u32 *)bitmap)[3];

Is this acceptable?

mrutland mentioned bitmap_to_u32array() but IIUC it is used to
copy an entire bitmap.


>>>> +	if (pos < MSI_COUNT)
>>>> +		writel(mask | BIT(pos), pcie->msi_mask);
>>>
>>> And it would make a lot more sense to move this write (which should be
>>> relaxed) to irq_unmask. Also, calling msi_mask for something that is an
>>> enable register is a bit counter intuitive.
>>
>> I don't have as much experience as you.
>> I just used the names in the HW documentation.
>> I think it is the "mask" (as in bitmap) of enabled MSIs.
>> I will change "mask" to "enable".
>>
>> Are you saying I should not use pci_msi_mask_irq and pci_msi_unmask_irq,
>> but register custom implementations? I should still call these in my
>> custom functions, right?
> 
> You can call both in your own mask/unmask methods. They serve different
> purpose (one is at the endpoint level, the other is at the MSI
> controller level).

So, if I understand correctly, I should check for an available MSIs
using the in-memory bitmap in tango_irq_domain_alloc(), but I would
defer actually enabling the MSI until irq_unmask?

I think work on bitmap and on the underlying HW regs need to be
protected under the same spinlock, correct?


>> Note: I don't have an "interrupt-map" prop because rev1 doesn't support
>> legacy PCI interrupts (INTx). But I see the PCI framework wrongly mapping
>> intA to my system's interrupt #1, presumably because I am lacking an
>> interrupt-map?
> 
> Probably. I don't think it is legal not to have an interrupt-map.

My understanding is that the interrupt-map actually specifies how to
map the legacy IRQs. My platform does not support legacy IRQs; maybe
there is some binding to say that? Maybe this is more a question for
the PCI folks.

Regards.

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web