[PATCH v3 0/2] Tango PCIe controller support

[PATCH v3 0/2] Tango PCIe controller support

[Marc Gonzalez]

Hello,

The patch is split in two parts, to ease review of two orthogonal
parts (MSI controller vs host bridge)

Marc Gonzalez (2):
  PCI: Add tango MSI controller support
  PCI: Add tango PCIe host bridge support

 Documentation/devicetree/bindings/pci/tango-pcie.txt |  34 ++++
 drivers/pci/host/Kconfig                             |   7 +
 drivers/pci/host/Makefile                            |   1 +
 drivers/pci/host/pcie-tango.c                        | 344 +++++++++++++++++++++++++++++++++++++++
 4 files changed, 386 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/pci/tango-pcie.txt
 create mode 100644 drivers/pci/host/pcie-tango.c

-- 
2.11.0

[PATCH v3 1/2] PCI: Add tango MSI controller support

[Marc Gonzalez]

The MSI controller in Tango supports 256 message-signaled interrupts,
and a single doorbell address.

Signed-off-by: Marc Gonzalez <marc_gonzalez@sigmadesigns.com>
---
Changes since v0.2
- Support 256 MSIs instead of only 32
- Use spinlock_t instead of struct mutex
- Add MSI_FLAG_PCI_MSIX flag

IRQs are acked in tango_msi_isr because handle_simple_irq leaves
ack, clear, mask and unmask up to the driver. For the same reason,
interrupt enable mask is updated from tango_irq_domain_alloc/free.
---
 drivers/pci/host/pcie-tango.c | 194 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 194 insertions(+)

diff --git a/drivers/pci/host/pcie-tango.c b/drivers/pci/host/pcie-tango.c
new file mode 100644
index 000000000000..e88850983a1d
--- /dev/null
+++ b/drivers/pci/host/pcie-tango.c
@@ -0,0 +1,194 @@
+#include <linux/module.h>
+#include <linux/irqchip/chained_irq.h>
+#include <linux/irqdomain.h>
+#include <linux/pci-ecam.h>
+#include <linux/msi.h>
+
+#define MSI_MAX 256
+
+struct tango_pcie {
+	DECLARE_BITMAP(bitmap, MSI_MAX);
+	spinlock_t lock;
+	void __iomem *mux;
+	void __iomem *msi_status;
+	void __iomem *msi_mask;
+	phys_addr_t msi_doorbell;
+	struct irq_domain *irq_domain;
+	struct irq_domain *msi_domain;
+	int irq;
+};
+
+/*** MSI CONTROLLER SUPPORT ***/
+
+static void dispatch(struct tango_pcie *pcie, unsigned long status, int base)
+{
+	unsigned int pos, virq;
+
+	for_each_set_bit(pos, &status, 32) {
+		virq = irq_find_mapping(pcie->irq_domain, base + pos);
+		generic_handle_irq(virq);
+	}
+}
+
+static void tango_msi_isr(struct irq_desc *desc)
+{
+	u32 status;
+	struct irq_chip *chip = irq_desc_get_chip(desc);
+	struct tango_pcie *pcie = irq_desc_get_handler_data(desc);
+	unsigned int base, offset, pos = 0;
+
+	chained_irq_enter(chip, desc);
+
+	while ((pos = find_next_bit(pcie->bitmap, MSI_MAX, pos)) < MSI_MAX) {
+		base = round_down(pos, 32);
+		offset = (pos / 32) * 4;
+		status = readl_relaxed(pcie->msi_status + offset);
+		writel_relaxed(status, pcie->msi_status + offset);
+		dispatch(pcie, status, base);
+		pos = base + 32;
+	}
+
+	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,
+};
+
+#define USE_DEF_OPS (MSI_FLAG_USE_DEF_DOM_OPS | MSI_FLAG_USE_DEF_CHIP_OPS)
+
+static struct msi_domain_info msi_domain_info = {
+	.flags	= USE_DEF_OPS | MSI_FLAG_PCI_MSIX,
+	.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 find_free_msi(struct irq_domain *dom, unsigned int virq)
+{
+	u32 val;
+	struct tango_pcie *pcie = dom->host_data;
+	unsigned int offset, pos;
+
+	pos = find_first_zero_bit(pcie->bitmap, MSI_MAX);
+	if (pos >= MSI_MAX)
+		return -ENOSPC;
+
+	offset = (pos / 32) * 4;
+	val = readl_relaxed(pcie->msi_mask + offset);
+	writel_relaxed(val | BIT(pos % 32), pcie->msi_mask + offset);
+	__set_bit(pos, pcie->bitmap);
+
+	irq_domain_set_info(dom, virq, pos, &tango_msi_chip,
+			dom->host_data, handle_simple_irq, NULL, NULL);
+
+	return 0;
+}
+
+static int tango_irq_domain_alloc(struct irq_domain *dom,
+		unsigned int virq, unsigned int nr_irqs, void *args)
+{
+	int err;
+	struct tango_pcie *pcie = dom->host_data;
+
+	spin_lock(&pcie->lock);
+	err = find_free_msi(dom, virq);
+	spin_unlock(&pcie->lock);
+
+	return err;
+}
+
+static void tango_irq_domain_free(struct irq_domain *dom,
+		unsigned int virq, unsigned int nr_irqs)
+{
+	u32 val;
+	struct irq_data *d = irq_domain_get_irq_data(dom, virq);
+	struct tango_pcie *pcie = irq_data_get_irq_chip_data(d);
+	unsigned int offset, pos = d->hwirq;
+
+	spin_lock(&pcie->lock);
+
+	offset = (pos / 32) * 4;
+	val = readl_relaxed(pcie->msi_mask + offset);
+	writel_relaxed(val & ~BIT(pos % 32), pcie->msi_mask + offset);
+	__clear_bit(pos, pcie->bitmap);
+
+	spin_unlock(&pcie->lock);
+}
+
+static const struct irq_domain_ops msi_dom_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_and_data(msi->irq, NULL, NULL);
+	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 i, virq;
+	struct fwnode_handle *fwnode = of_node_to_fwnode(pdev->dev.of_node);
+	struct irq_domain *msi_dom, *irq_dom;
+
+	spin_lock_init(&pcie->lock);
+
+	for (i = 0; i < MSI_MAX / 32; ++i)
+		writel_relaxed(0, pcie->msi_mask + i * 4);
+
+	irq_dom = irq_domain_create_linear(fwnode, MSI_MAX, &msi_dom_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) {
+		pr_err("Failed to map IRQ\n");
+		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;
+}
-- 
2.11.0

Re: [PATCH v3 1/2] PCI: Add tango MSI controller support

[Marc Zyngier]

On 29/03/17 12:29, Marc Gonzalez wrote:
> The MSI controller in Tango supports 256 message-signaled interrupts,
> and a single doorbell address.
> 
> Signed-off-by: Marc Gonzalez <marc_gonzalez@sigmadesigns.com>
> ---
> Changes since v0.2
> - Support 256 MSIs instead of only 32
> - Use spinlock_t instead of struct mutex
> - Add MSI_FLAG_PCI_MSIX flag
> 
> IRQs are acked in tango_msi_isr because handle_simple_irq leaves
> ack, clear, mask and unmask up to the driver. For the same reason,
> interrupt enable mask is updated from tango_irq_domain_alloc/free.

I've asked you to move this to individual methods. You've decided not
to, and that's your call. But I now wonder why I'm even bothering to
review this, as you've so far just wasted my time.

Anyway...

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

How is that include relevant to this patch?

> +#include <linux/msi.h>
> +
> +#define MSI_MAX 256
> +
> +struct tango_pcie {
> +	DECLARE_BITMAP(bitmap, MSI_MAX);
> +	spinlock_t lock;
> +	void __iomem *mux;
> +	void __iomem *msi_status;
> +	void __iomem *msi_mask;
> +	phys_addr_t msi_doorbell;
> +	struct irq_domain *irq_domain;
> +	struct irq_domain *msi_domain;
> +	int irq;
> +};
> +
> +/*** MSI CONTROLLER SUPPORT ***/
> +
> +static void dispatch(struct tango_pcie *pcie, unsigned long status, int base)
> +{
> +	unsigned int pos, virq;
> +
> +	for_each_set_bit(pos, &status, 32) {
> +		virq = irq_find_mapping(pcie->irq_domain, base + pos);
> +		generic_handle_irq(virq);
> +	}
> +}
> +
> +static void tango_msi_isr(struct irq_desc *desc)
> +{
> +	u32 status;
> +	struct irq_chip *chip = irq_desc_get_chip(desc);
> +	struct tango_pcie *pcie = irq_desc_get_handler_data(desc);
> +	unsigned int base, offset, pos = 0;
> +
> +	chained_irq_enter(chip, desc);
> +
> +	while ((pos = find_next_bit(pcie->bitmap, MSI_MAX, pos)) < MSI_MAX) {
> +		base = round_down(pos, 32);
> +		offset = (pos / 32) * 4;
> +		status = readl_relaxed(pcie->msi_status + offset);
> +		writel_relaxed(status, pcie->msi_status + offset);
> +		dispatch(pcie, status, base);
> +		pos = base + 32;
> +	}
> +
> +	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,

How do you make that work if the PCI device doesn't support per-MSI masking?

> +};
> +
> +#define USE_DEF_OPS (MSI_FLAG_USE_DEF_DOM_OPS | MSI_FLAG_USE_DEF_CHIP_OPS)

How is that useful?

> +
> +static struct msi_domain_info msi_domain_info = {
> +	.flags	= USE_DEF_OPS | MSI_FLAG_PCI_MSIX,
> +	.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 find_free_msi(struct irq_domain *dom, unsigned int virq)
> +{
> +	u32 val;
> +	struct tango_pcie *pcie = dom->host_data;
> +	unsigned int offset, pos;
> +
> +	pos = find_first_zero_bit(pcie->bitmap, MSI_MAX);
> +	if (pos >= MSI_MAX)
> +		return -ENOSPC;
> +
> +	offset = (pos / 32) * 4;
> +	val = readl_relaxed(pcie->msi_mask + offset);
> +	writel_relaxed(val | BIT(pos % 32), pcie->msi_mask + offset);

Great. I'm now in a position where I can take an interrupt (because of
the broken locking that doesn't disable interrupts), but the bitmap
doesn't indicate it yet. With a bit of luck, I'll never make any forward
progress.

> +	__set_bit(pos, pcie->bitmap);
> +
> +	irq_domain_set_info(dom, virq, pos, &tango_msi_chip,
> +			dom->host_data, handle_simple_irq, NULL, NULL);

I've told you a number of times that PCI MSIs are edge triggered...

> +
> +	return 0;
> +}
> +
> +static int tango_irq_domain_alloc(struct irq_domain *dom,
> +		unsigned int virq, unsigned int nr_irqs, void *args)
> +{
> +	int err;
> +	struct tango_pcie *pcie = dom->host_data;
> +
> +	spin_lock(&pcie->lock);
> +	err = find_free_msi(dom, virq);
> +	spin_unlock(&pcie->lock);
> +
> +	return err;
> +}
> +
> +static void tango_irq_domain_free(struct irq_domain *dom,
> +		unsigned int virq, unsigned int nr_irqs)
> +{
> +	u32 val;
> +	struct irq_data *d = irq_domain_get_irq_data(dom, virq);
> +	struct tango_pcie *pcie = irq_data_get_irq_chip_data(d);
> +	unsigned int offset, pos = d->hwirq;
> +
> +	spin_lock(&pcie->lock);
> +
> +	offset = (pos / 32) * 4;
> +	val = readl_relaxed(pcie->msi_mask + offset);
> +	writel_relaxed(val & ~BIT(pos % 32), pcie->msi_mask + offset);
> +	__clear_bit(pos, pcie->bitmap);
> +
> +	spin_unlock(&pcie->lock);
> +}
> +
> +static const struct irq_domain_ops msi_dom_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);

Be consistent in your naming. It's called pcie everywhere else.

> +
> +	irq_set_chained_handler_and_data(msi->irq, NULL, NULL);
> +	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 i, virq;
> +	struct fwnode_handle *fwnode = of_node_to_fwnode(pdev->dev.of_node);
> +	struct irq_domain *msi_dom, *irq_dom;
> +
> +	spin_lock_init(&pcie->lock);
> +
> +	for (i = 0; i < MSI_MAX / 32; ++i)
> +		writel_relaxed(0, pcie->msi_mask + i * 4);
> +
> +	irq_dom = irq_domain_create_linear(fwnode, MSI_MAX, &msi_dom_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) {
> +		pr_err("Failed to map IRQ\n");
> +		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;
> +}
> 

So there is not much progress from the previous version. It is just
broken in a different ways, and ignores most of the work that is already
done in the irqchip core. I can only repeat what I've said in my
previous review.

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

Re: [PATCH v3 1/2] PCI: Add tango MSI controller support

[Mason]

On 29/03/2017 14:29, Marc Zyngier wrote:

> On 29/03/17 12:29, Marc Gonzalez wrote:
>
>> The MSI controller in Tango supports 256 message-signaled interrupts,
>> and a single doorbell address.
>>
>> Signed-off-by: Marc Gonzalez <marc_gonzalez@sigmadesigns.com>
>> ---
>> Changes since v0.2
>> - Support 256 MSIs instead of only 32
>> - Use spinlock_t instead of struct mutex
>> - Add MSI_FLAG_PCI_MSIX flag
>>
>> IRQs are acked in tango_msi_isr because handle_simple_irq leaves
>> ack, clear, mask and unmask up to the driver. For the same reason,
>> interrupt enable mask is updated from tango_irq_domain_alloc/free.
> 
> I've asked you to move this to individual methods. You've decided not
> to, and that's your call. But I now wonder why I'm even bothering to
> review this, as you've so far just wasted my time.

I misunderstood what you wrote. When you pointed out the comment at
the top of handle_simple_irq (which I mentioned in my above blurb)
I took that to mean that I had to follow those instructions.

Judging by what you wrote below, I must replace handle_simple_irq
with handle_edge_irq, which will call the irq_chip callbacks.

But I don't understand how to get my pcie pointer back in irq_ack
or irq_unmask, or the relevant msi. Can you throw me a clue?

>> +static struct irq_chip tango_msi_irq_chip = {
>> +	.name = "MSI",
>> +	.irq_mask = pci_msi_mask_irq,
>> +	.irq_unmask = pci_msi_unmask_irq,
> 
> How do you make that work if the PCI device doesn't support per-MSI masking?

It seems you're saying this code is broken. Is it functional
in the Altera driver, and I did something to break it?

>> +static int find_free_msi(struct irq_domain *dom, unsigned int virq)
>> +{
>> +	u32 val;
>> +	struct tango_pcie *pcie = dom->host_data;
>> +	unsigned int offset, pos;
>> +
>> +	pos = find_first_zero_bit(pcie->bitmap, MSI_MAX);
>> +	if (pos >= MSI_MAX)
>> +		return -ENOSPC;
>> +
>> +	offset = (pos / 32) * 4;
>> +	val = readl_relaxed(pcie->msi_mask + offset);
>> +	writel_relaxed(val | BIT(pos % 32), pcie->msi_mask + offset);
> 
> Great. I'm now in a position where I can take an interrupt (because of
> the broken locking that doesn't disable interrupts), but the bitmap
> doesn't indicate it yet. With a bit of luck, I'll never make any forward
> progress.

Is this the Yoda way to say:
"Hey moron, use spin_lock_irqsave instead of spin_lock"?

>> +	irq_domain_set_info(dom, virq, pos, &tango_msi_chip,
>> +			dom->host_data, handle_simple_irq, NULL, NULL);
> 
> I've told you a number of times that PCI MSIs are edge triggered...

I will register handle_edge_irq.

> So there is not much progress from the previous version. It is just
> broken in a different ways, and ignores most of the work that is already
> done in the irqchip core.

I wish nothing more than to be able to use as much infrastructure
as possible, in order to write as little code as possible.

Regards.

Re: [PATCH v3 1/2] PCI: Add tango MSI controller support

[Mason]

On 29/03/2017 15:16, Mason wrote:

> But I don't understand how to get my pcie pointer back in irq_ack
> or irq_unmask, or the relevant msi. Can you throw me a clue?

Let's see... the irq_chip call-backs receive an irq_data pointer.

struct irq_data - per irq chip data passed down to chip functions
struct irq_chip - hardware interrupt chip descriptor

irq_data contains a void *chip_data member.
Can I use chip_data to stash a struct tango_pcie pointer?

/**
 *	irq_set_chip_data - set irq chip data for an irq
 *	@irq:	Interrupt number
 *	@data:	Pointer to chip specific data
 *
 *	Set the hardware irq chip data for an irq
 */
int irq_set_chip_data(unsigned int irq, void *data)

But where would I call it? And for which irq?


Otherwise, I found by trial-and-error that I can reach pcie through

data->domain->parent->host_data

data->domain == msi_dom
msi_dom->parent == irq_dom
irq_dom->host_data == pcie

But data->irq and data->hwirq don't hold the MSI index :-(

I suppose I have to ask for some mapping?

Regards.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v3 2/2] PCI: Add tango PCIe host bridge support

[Marc Gonzalez]

This driver is used to work around HW bugs in the controller.

Note: the controller does NOT support the following features.

  Legacy PCI interrupts
  IO space

Signed-off-by: Marc Gonzalez <marc_gonzalez@sigmadesigns.com>
---
 Documentation/devicetree/bindings/pci/tango-pcie.txt |  33 +++++++++
 drivers/pci/host/Kconfig                             |   7 ++
 drivers/pci/host/Makefile                            |   1 +
 drivers/pci/host/pcie-tango.c                        | 150 +++++++++++++++++++++++++++++++++++++++
 4 files changed, 191 insertions(+)

diff --git a/Documentation/devicetree/bindings/pci/tango-pcie.txt b/Documentation/devicetree/bindings/pci/tango-pcie.txt
new file mode 100644
index 000000000000..f8e150ec41d8
--- /dev/null
+++ b/Documentation/devicetree/bindings/pci/tango-pcie.txt
@@ -0,0 +1,33 @@
+Sigma Designs Tango PCIe controller
+
+Required properties:
+
+- compatible: "sigma,smp8759-pcie"
+- reg: address/size of PCI configuration space, and pcie_reg
+- bus-range: defined by size of PCI configuration space
+- device_type: "pci"
+- #size-cells: <2>
+- #address-cells: <3>
+- #interrupt-cells: <1>
+- ranges: translation from system to bus addresses
+- interrupts: spec for misc interrupts and MSI
+- msi-controller
+
+Example:
+
+	pcie@2e000 {
+		compatible = "sigma,smp8759-pcie";
+		reg = <0x50000000 SZ_64M>, <0x2e000 0x100>;
+		bus-range = <0 63>;
+		device_type = "pci";
+		#size-cells = <2>;
+		#address-cells = <3>;
+		#interrupt-cells = <1>;
+		/* http://elinux.org/Device_Tree_Usage#PCI_Address_Translation */
+			/* BUS_ADDRESS(3)  CPU_PHYSICAL(1)  SIZE(2) */
+		ranges = <0x02000000 0x0 0x04000000  0x54000000  0x0 SZ_192M>;
+		interrupts =
+			<54 IRQ_TYPE_LEVEL_HIGH>, /* misc interrupts */
+			<55 IRQ_TYPE_LEVEL_HIGH>; /* MSI */
+		msi-controller;
+	};
diff --git a/drivers/pci/host/Kconfig b/drivers/pci/host/Kconfig
index d7e7c0a827c3..8a622578a760 100644
--- a/drivers/pci/host/Kconfig
+++ b/drivers/pci/host/Kconfig
@@ -285,6 +285,13 @@ config PCIE_ROCKCHIP
 	  There is 1 internal PCIe port available to support GEN2 with
 	  4 slots.
 
+config PCIE_TANGO
+	tristate "Tango PCIe controller"
+	depends on ARCH_TANGO && PCI_MSI && OF
+	select PCI_HOST_COMMON
+	help
+	  Say Y here to enable PCIe controller support on Tango SoC.
+
 config VMD
 	depends on PCI_MSI && X86_64
 	tristate "Intel Volume Management Device Driver"
diff --git a/drivers/pci/host/Makefile b/drivers/pci/host/Makefile
index 084cb4983645..fc7ea90196f3 100644
--- a/drivers/pci/host/Makefile
+++ b/drivers/pci/host/Makefile
@@ -32,4 +32,5 @@ obj-$(CONFIG_PCI_HOST_THUNDER_PEM) += pci-thunder-pem.o
 obj-$(CONFIG_PCIE_ARMADA_8K) += pcie-armada8k.o
 obj-$(CONFIG_PCIE_ARTPEC6) += pcie-artpec6.o
 obj-$(CONFIG_PCIE_ROCKCHIP) += pcie-rockchip.o
+obj-$(CONFIG_PCIE_TANGO) += pcie-tango.o
 obj-$(CONFIG_VMD) += vmd.o
diff --git a/drivers/pci/host/pcie-tango.c b/drivers/pci/host/pcie-tango.c
index e88850983a1d..dbc2663486b5 100644
--- a/drivers/pci/host/pcie-tango.c
+++ b/drivers/pci/host/pcie-tango.c
@@ -192,3 +192,153 @@ static int tango_msi_probe(struct platform_device *pdev, struct tango_pcie *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 = ~0; /* Is this required even if we return an error? */
+		return PCIBIOS_FUNC_NOT_SUPPORTED; /* Error seems appropriate */
+	}
+
+	/*
+	 * 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_relaxed(1, pcie->mux);
+	ret = pci_generic_config_read(bus, devfn, where, size, val);
+	writel_relaxed(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_relaxed(1, pcie->mux);
+	ret = pci_generic_config_write(bus, devfn, where, size, val);
+	writel_relaxed(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" },
+	{ /* 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 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);
+
+	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,
+	},
+};
+
+module_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

Re: [PATCH v3 2/2] PCI: Add tango PCIe host bridge support

[Robin Murphy]

On 29/03/17 12:34, Marc Gonzalez wrote:
> This driver is used to work around HW bugs in the controller.
> 
> Note: the controller does NOT support the following features.
> 
>   Legacy PCI interrupts
>   IO space
> 
> Signed-off-by: Marc Gonzalez <marc_gonzalez-y1yR0Z3OICC7zZZRDBGcUA@public.gmane.org>
> ---
>  Documentation/devicetree/bindings/pci/tango-pcie.txt |  33 +++++++++
>  drivers/pci/host/Kconfig                             |   7 ++
>  drivers/pci/host/Makefile                            |   1 +
>  drivers/pci/host/pcie-tango.c                        | 150 +++++++++++++++++++++++++++++++++++++++
>  4 files changed, 191 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/pci/tango-pcie.txt b/Documentation/devicetree/bindings/pci/tango-pcie.txt
> new file mode 100644
> index 000000000000..f8e150ec41d8
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/pci/tango-pcie.txt
> @@ -0,0 +1,33 @@
> +Sigma Designs Tango PCIe controller
> +
> +Required properties:
> +
> +- compatible: "sigma,smp8759-pcie"
> +- reg: address/size of PCI configuration space, and pcie_reg
> +- bus-range: defined by size of PCI configuration space
> +- device_type: "pci"
> +- #size-cells: <2>
> +- #address-cells: <3>
> +- #interrupt-cells: <1>
> +- ranges: translation from system to bus addresses
> +- interrupts: spec for misc interrupts and MSI
> +- msi-controller
> +
> +Example:
> +
> +	pcie@2e000 {
> +		compatible = "sigma,smp8759-pcie";
> +		reg = <0x50000000 SZ_64M>, <0x2e000 0x100>;
> +		bus-range = <0 63>;
> +		device_type = "pci";
> +		#size-cells = <2>;
> +		#address-cells = <3>;
> +		#interrupt-cells = <1>;
> +		/* http://elinux.org/Device_Tree_Usage#PCI_Address_Translation */
> +			/* BUS_ADDRESS(3)  CPU_PHYSICAL(1)  SIZE(2) */
> +		ranges = <0x02000000 0x0 0x04000000  0x54000000  0x0 SZ_192M>;
> +		interrupts =
> +			<54 IRQ_TYPE_LEVEL_HIGH>, /* misc interrupts */
> +			<55 IRQ_TYPE_LEVEL_HIGH>; /* MSI */
> +		msi-controller;
> +	};
> diff --git a/drivers/pci/host/Kconfig b/drivers/pci/host/Kconfig
> index d7e7c0a827c3..8a622578a760 100644
> --- a/drivers/pci/host/Kconfig
> +++ b/drivers/pci/host/Kconfig
> @@ -285,6 +285,13 @@ config PCIE_ROCKCHIP
>  	  There is 1 internal PCIe port available to support GEN2 with
>  	  4 slots.
>  
> +config PCIE_TANGO
> +	tristate "Tango PCIe controller"
> +	depends on ARCH_TANGO && PCI_MSI && OF
> +	select PCI_HOST_COMMON
> +	help
> +	  Say Y here to enable PCIe controller support on Tango SoC.

Nit: in line with the other help texts, that could probably benefit from
a wee bit more context (i.e. "Sigma Designs Tango SoCs")

> +
>  config VMD
>  	depends on PCI_MSI && X86_64
>  	tristate "Intel Volume Management Device Driver"
> diff --git a/drivers/pci/host/Makefile b/drivers/pci/host/Makefile
> index 084cb4983645..fc7ea90196f3 100644
> --- a/drivers/pci/host/Makefile
> +++ b/drivers/pci/host/Makefile
> @@ -32,4 +32,5 @@ obj-$(CONFIG_PCI_HOST_THUNDER_PEM) += pci-thunder-pem.o
>  obj-$(CONFIG_PCIE_ARMADA_8K) += pcie-armada8k.o
>  obj-$(CONFIG_PCIE_ARTPEC6) += pcie-artpec6.o
>  obj-$(CONFIG_PCIE_ROCKCHIP) += pcie-rockchip.o
> +obj-$(CONFIG_PCIE_TANGO) += pcie-tango.o
>  obj-$(CONFIG_VMD) += vmd.o
> diff --git a/drivers/pci/host/pcie-tango.c b/drivers/pci/host/pcie-tango.c
> index e88850983a1d..dbc2663486b5 100644
> --- a/drivers/pci/host/pcie-tango.c
> +++ b/drivers/pci/host/pcie-tango.c
> @@ -192,3 +192,153 @@ static int tango_msi_probe(struct platform_device *pdev, struct tango_pcie *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 = ~0; /* Is this required even if we return an error? */
> +		return PCIBIOS_FUNC_NOT_SUPPORTED; /* Error seems appropriate */
> +	}
> +
> +	/*
> +	 * 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.
> +	 */

What about David's suggestion of using an IPI for safe mutual exclusion?

> +	writel_relaxed(1, pcie->mux);
> +	ret = pci_generic_config_read(bus, devfn, where, size, val);
> +	writel_relaxed(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_relaxed(1, pcie->mux);
> +	ret = pci_generic_config_write(bus, devfn, where, size, val);
> +	writel_relaxed(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" },

General tip: use your .data member here...

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

...then retrieve it with of_device_get_match_data() here. No need to
reinvent the wheel (or have to worry about the ordering of multiple
compatibles once rev. n+1 comes around).

> +
> +	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,
> +	},
> +};
> +
> +module_platform_driver(tango_pcie_driver);
> +
> +#define VENDOR_SIGMA	0x1105

Should this not be in include/linux/pci_ids.h?

Robin.

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

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 2/2] PCI: Add tango PCIe host bridge support

[Mason]

On 29/03/2017 14:19, Robin Murphy wrote:

> On 29/03/17 12:34, Marc Gonzalez wrote:
> 
>> +	/*
>> +	 * 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.
>> +	 */
> 
> What about David's suggestion of using an IPI for safe mutual exclusion?

I was left with the impression that this wouldn't solve the problem.
If a mem space access is "in flight" on core0 when core1 starts a
config space access, an IPI will not prevent breakage.

Did I misunderstand?

For my education, what is the API to send an IPI?
And the API to handle an IPI?

>> +	if (of_device_is_compatible(dev->of_node, "sigma,smp8759-pcie"))
>> +		smp8759_init(pcie, base);
> 
> ...then retrieve it with of_device_get_match_data() here. No need to
> reinvent the wheel (or have to worry about the ordering of multiple
> compatibles once rev. n+1 comes around).

I actually asked about this on IRC. The consensus was "use what
best fits your use case". I need to do some processing based on
the revision, so I thought

  if (chip_x)
	do_chip_x_init()

was a good way to express my intent. Did I misunderstand?

For example, the init function for rev2 currently looks like this:

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

>> +#define VENDOR_SIGMA	0x1105
> 
> Should this not be in include/linux/pci_ids.h?

Doh! Very likely. Thanks.

Regards.

Re: [PATCH v3 2/2] PCI: Add tango PCIe host bridge support

[Robin Murphy]

On 29/03/17 13:53, Mason wrote:
> On 29/03/2017 14:19, Robin Murphy wrote:
> 
>> On 29/03/17 12:34, Marc Gonzalez wrote:
>>
>>> +	/*
>>> +	 * 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.
>>> +	 */
>>
>> What about David's suggestion of using an IPI for safe mutual exclusion?
> 
> I was left with the impression that this wouldn't solve the problem.
> If a mem space access is "in flight" on core0 when core1 starts a
> config space access, an IPI will not prevent breakage.
> 
> Did I misunderstand?
> 
> For my education, what is the API to send an IPI?
> And the API to handle an IPI?

There are a few ways you could implement some custom cross-call,
although in this case I think stop_machine() would probably be the most
appropriate candidate. However, you're right that in general it may not
actually help enough to be worthwhile - a DSB SY would ensure that
in-flight transactions have at least been observed by the CPUs and any
other coherent masters, but for any writes with a memory type allowing
early acknowledgement (i.e. a Normal or Device mapping of a BAR) that
doesn't necessarily correlate with them having reached their ultimate
destination. For a PCI destination in particular, I think the normal way
to ensure all posted writes have completed would be to read from config
space; ah...

>>> +	if (of_device_is_compatible(dev->of_node, "sigma,smp8759-pcie"))
>>> +		smp8759_init(pcie, base);
>>
>> ...then retrieve it with of_device_get_match_data() here. No need to
>> reinvent the wheel (or have to worry about the ordering of multiple
>> compatibles once rev. n+1 comes around).
> 
> I actually asked about this on IRC. The consensus was "use what
> best fits your use case". I need to do some processing based on
> the revision, so I thought
> 
>   if (chip_x)
> 	do_chip_x_init()
> 
> was a good way to express my intent. Did I misunderstand?

No, I'm in no way disputing that; what I'm pointing out is that you
already have an explicitly provided way to associate a value of "chip_x"
with a given compatible string - see other callers of
of_device_get_match_data() for inspiration. I don't have much of an
opinion as to whether it's an enum, a static structure of offsets and
callbacks, or you embrace the nasal demons and just wedge the init
function pointer in there directly (this'll never run on
IA-64/M68k/etc., right? :P). The point is that not only is it cleaner
and scales better as the driver grows, it stops you having to worry at
all about setting this trap for yourself:

	compatible = "rev3-with-extra-fun", "rev3";
	...

	if (of_device_is_compatible(dev, "rev3"))
		boring_init_without_extra_fun();	/* :( */

because once you've made your code robust against that, you'll realise
that what you've done is wasted your time open-coding a creaky
approximation of of_match_device().

Robin.

> For example, the init function for rev2 currently looks like this:
> 
> 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);
> }
> 
>>> +#define VENDOR_SIGMA	0x1105
>>
>> Should this not be in include/linux/pci_ids.h?
> 
> Doh! Very likely. Thanks.
> 
> Regards.
> 

RE: [PATCH v3 2/2] PCI: Add tango PCIe host bridge support

[David Laight]

> > For my education, what is the API to send an IPI?
> > And the API to handle an IPI?
> 
> There are a few ways you could implement some custom cross-call,
> although in this case I think stop_machine() would probably be the most
> appropriate candidate. However, you're right that in general it may not
> actually help enough to be worthwhile - a DSB SY would ensure that
> in-flight transactions have at least been observed by the CPUs and any
> other coherent masters, but for any writes with a memory type allowing
> early acknowledgement (i.e. a Normal or Device mapping of a BAR) that
> doesn't necessarily correlate with them having reached their ultimate
> destination. For a PCI destination in particular, I think the normal way
> to ensure all posted writes have completed would be to read from config
> space; ah...

He almost certainly doesn't need to wait for the cycle to complete,
just long enough for the cycle to have been sent.

	David

Re: [PATCH v3 2/2] PCI: Add tango PCIe host bridge support

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 19079 bytes --]

Hi Marc,

[auto build test ERROR on v4.9-rc8]
[also build test ERROR on next-20170330]
[cannot apply to pci/next]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Marc-Gonzalez/Tango-PCIe-controller-support/20170330-154028
config: arm-allmodconfig (attached as .config)
compiler: arm-linux-gnueabi-gcc (Debian 6.1.1-9) 6.1.1 20160705
reproduce:
        wget https://raw.githubusercontent.com/01org/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        make.cross ARCH=arm 

All error/warnings (new ones prefixed by >>):

   In file included from drivers/pci/host/pcie-tango.c:4:0:
>> include/linux/pci-ecam.h:29:19: error: field 'pci_ops' has incomplete type
     struct pci_ops   pci_ops;
                      ^~~~~~~
>> include/linux/pci-ecam.h:57:39: warning: 'struct pci_bus' declared inside parameter list will not be visible outside of this definition or declaration
    void __iomem *pci_ecam_map_bus(struct pci_bus *bus, unsigned int devfn,
                                          ^~~~~~~
>> drivers/pci/host/pcie-tango.c:198:39: warning: 'struct pci_bus' declared inside parameter list will not be visible outside of this definition or declaration
    static int smp8759_config_read(struct pci_bus *bus,
                                          ^~~~~~~
   drivers/pci/host/pcie-tango.c: In function 'smp8759_config_read':
>> drivers/pci/host/pcie-tango.c:202:37: error: dereferencing pointer to incomplete type 'struct pci_bus'
     struct pci_config_window *cfg = bus->sysdata;
                                        ^~
>> drivers/pci/host/pcie-tango.c:211:10: error: 'PCIBIOS_FUNC_NOT_SUPPORTED' undeclared (first use in this function)
      return PCIBIOS_FUNC_NOT_SUPPORTED; /* Error seems appropriate */
             ^~~~~~~~~~~~~~~~~~~~~~~~~~
   drivers/pci/host/pcie-tango.c:211:10: note: each undeclared identifier is reported only once for each function it appears in
>> drivers/pci/host/pcie-tango.c:219:15: error: 'PCI_BASE_ADDRESS_0' undeclared (first use in this function)
     if (where == PCI_BASE_ADDRESS_0 && bus->number == 0) {
                  ^~~~~~~~~~~~~~~~~~
>> drivers/pci/host/pcie-tango.c:221:10: error: 'PCIBIOS_SUCCESSFUL' undeclared (first use in this function)
      return PCIBIOS_SUCCESSFUL; /* Should we return error or success? */
             ^~~~~~~~~~~~~~~~~~
>> drivers/pci/host/pcie-tango.c:233:8: error: implicit declaration of function 'pci_generic_config_read' [-Werror=implicit-function-declaration]
     ret = pci_generic_config_read(bus, devfn, where, size, val);
           ^~~~~~~~~~~~~~~~~~~~~~~
   drivers/pci/host/pcie-tango.c: At top level:
   drivers/pci/host/pcie-tango.c:239:40: warning: 'struct pci_bus' declared inside parameter list will not be visible outside of this definition or declaration
    static int smp8759_config_write(struct pci_bus *bus,
                                           ^~~~~~~
   drivers/pci/host/pcie-tango.c: In function 'smp8759_config_write':
   drivers/pci/host/pcie-tango.c:243:37: error: dereferencing pointer to incomplete type 'struct pci_bus'
     struct pci_config_window *cfg = bus->sysdata;
                                        ^~
>> drivers/pci/host/pcie-tango.c:247:8: error: implicit declaration of function 'pci_generic_config_write' [-Werror=implicit-function-declaration]
     ret = pci_generic_config_write(bus, devfn, where, size, val);
           ^~~~~~~~~~~~~~~~~~~~~~~~
   drivers/pci/host/pcie-tango.c: At top level:
>> drivers/pci/host/pcie-tango.c:256:3: error: field name not in record or union initializer
      .map_bus = pci_ecam_map_bus,
      ^
   drivers/pci/host/pcie-tango.c:256:3: note: (near initialization for 'smp8759_ecam_ops.pci_ops')
   drivers/pci/host/pcie-tango.c:257:3: error: field name not in record or union initializer
      .read  = smp8759_config_read,
      ^
   drivers/pci/host/pcie-tango.c:257:3: note: (near initialization for 'smp8759_ecam_ops.pci_ops')
   drivers/pci/host/pcie-tango.c:258:3: error: field name not in record or union initializer
      .write  = smp8759_config_write,
      ^
   drivers/pci/host/pcie-tango.c:258:3: note: (near initialization for 'smp8759_ecam_ops.pci_ops')
   drivers/pci/host/pcie-tango.c: In function 'tango_fixup_class':
>> drivers/pci/host/pcie-tango.c:329:5: error: dereferencing pointer to incomplete type 'struct pci_dev'
     dev->class = PCI_CLASS_BRIDGE_PCI << 8;
        ^~
>> drivers/pci/host/pcie-tango.c:329:15: error: 'PCI_CLASS_BRIDGE_PCI' undeclared (first use in this function)
     dev->class = PCI_CLASS_BRIDGE_PCI << 8;
                  ^~~~~~~~~~~~~~~~~~~~
   drivers/pci/host/pcie-tango.c: At top level:
>> drivers/pci/host/pcie-tango.c:320:22: error: expected declaration specifiers or '...' before numeric constant
    #define VENDOR_SIGMA 0x1105
                         ^
>> drivers/pci/host/pcie-tango.c:331:25: note: in expansion of macro 'VENDOR_SIGMA'
    DECLARE_PCI_FIXUP_EARLY(VENDOR_SIGMA, PCI_ANY_ID, tango_fixup_class);
                            ^~~~~~~~~~~~
   In file included from include/linux/of.h:22:0,
                    from include/linux/irqdomain.h:34,
                    from drivers/pci/host/pcie-tango.c:3:
>> include/linux/mod_devicetable.h:16:20: error: expected declaration specifiers or '...' before '(' token
    #define PCI_ANY_ID (~0)
                       ^
>> drivers/pci/host/pcie-tango.c:331:39: note: in expansion of macro 'PCI_ANY_ID'
    DECLARE_PCI_FIXUP_EARLY(VENDOR_SIGMA, PCI_ANY_ID, tango_fixup_class);
                                          ^~~~~~~~~~
>> drivers/pci/host/pcie-tango.c:331:51: error: expected declaration specifiers or '...' before 'tango_fixup_class'
    DECLARE_PCI_FIXUP_EARLY(VENDOR_SIGMA, PCI_ANY_ID, tango_fixup_class);
                                                      ^~~~~~~~~~~~~~~~~
   drivers/pci/host/pcie-tango.c: In function 'tango_fixup_bar':
>> drivers/pci/host/pcie-tango.c:342:9: error: implicit declaration of function 'pci_write_config_dword' [-Werror=implicit-function-declaration]
            pci_write_config_dword(dev, PCI_BASE_ADDRESS_0, 0x80000000);
            ^~~~~~~~~~~~~~~~~~~~~~
   drivers/pci/host/pcie-tango.c:342:37: error: 'PCI_BASE_ADDRESS_0' undeclared (first use in this function)
            pci_write_config_dword(dev, PCI_BASE_ADDRESS_0, 0x80000000);
                                        ^~~~~~~~~~~~~~~~~~
   drivers/pci/host/pcie-tango.c: At top level:
>> drivers/pci/host/pcie-tango.c:320:22: error: expected declaration specifiers or '...' before numeric constant
    #define VENDOR_SIGMA 0x1105
                         ^
   drivers/pci/host/pcie-tango.c:344:25: note: in expansion of macro 'VENDOR_SIGMA'
    DECLARE_PCI_FIXUP_FINAL(VENDOR_SIGMA, PCI_ANY_ID, tango_fixup_bar);
                            ^~~~~~~~~~~~
   In file included from include/linux/of.h:22:0,
                    from include/linux/irqdomain.h:34,
                    from drivers/pci/host/pcie-tango.c:3:
>> include/linux/mod_devicetable.h:16:20: error: expected declaration specifiers or '...' before '(' token
    #define PCI_ANY_ID (~0)
                       ^
   drivers/pci/host/pcie-tango.c:344:39: note: in expansion of macro 'PCI_ANY_ID'
    DECLARE_PCI_FIXUP_FINAL(VENDOR_SIGMA, PCI_ANY_ID, tango_fixup_bar);
                                          ^~~~~~~~~~
--
   In file included from drivers/pci//host/pcie-tango.c:4:0:
>> include/linux/pci-ecam.h:29:19: error: field 'pci_ops' has incomplete type
     struct pci_ops   pci_ops;
                      ^~~~~~~
>> include/linux/pci-ecam.h:57:39: warning: 'struct pci_bus' declared inside parameter list will not be visible outside of this definition or declaration
    void __iomem *pci_ecam_map_bus(struct pci_bus *bus, unsigned int devfn,
                                          ^~~~~~~
   drivers/pci//host/pcie-tango.c:198:39: warning: 'struct pci_bus' declared inside parameter list will not be visible outside of this definition or declaration
    static int smp8759_config_read(struct pci_bus *bus,
                                          ^~~~~~~
   drivers/pci//host/pcie-tango.c: In function 'smp8759_config_read':
   drivers/pci//host/pcie-tango.c:202:37: error: dereferencing pointer to incomplete type 'struct pci_bus'
     struct pci_config_window *cfg = bus->sysdata;
                                        ^~
   drivers/pci//host/pcie-tango.c:211:10: error: 'PCIBIOS_FUNC_NOT_SUPPORTED' undeclared (first use in this function)
      return PCIBIOS_FUNC_NOT_SUPPORTED; /* Error seems appropriate */
             ^~~~~~~~~~~~~~~~~~~~~~~~~~
   drivers/pci//host/pcie-tango.c:211:10: note: each undeclared identifier is reported only once for each function it appears in
   drivers/pci//host/pcie-tango.c:219:15: error: 'PCI_BASE_ADDRESS_0' undeclared (first use in this function)
     if (where == PCI_BASE_ADDRESS_0 && bus->number == 0) {
                  ^~~~~~~~~~~~~~~~~~
   drivers/pci//host/pcie-tango.c:221:10: error: 'PCIBIOS_SUCCESSFUL' undeclared (first use in this function)
      return PCIBIOS_SUCCESSFUL; /* Should we return error or success? */
             ^~~~~~~~~~~~~~~~~~
   drivers/pci//host/pcie-tango.c:233:8: error: implicit declaration of function 'pci_generic_config_read' [-Werror=implicit-function-declaration]
     ret = pci_generic_config_read(bus, devfn, where, size, val);
           ^~~~~~~~~~~~~~~~~~~~~~~
   drivers/pci//host/pcie-tango.c: At top level:
   drivers/pci//host/pcie-tango.c:239:40: warning: 'struct pci_bus' declared inside parameter list will not be visible outside of this definition or declaration
    static int smp8759_config_write(struct pci_bus *bus,
                                           ^~~~~~~
   drivers/pci//host/pcie-tango.c: In function 'smp8759_config_write':
   drivers/pci//host/pcie-tango.c:243:37: error: dereferencing pointer to incomplete type 'struct pci_bus'
     struct pci_config_window *cfg = bus->sysdata;
                                        ^~
   drivers/pci//host/pcie-tango.c:247:8: error: implicit declaration of function 'pci_generic_config_write' [-Werror=implicit-function-declaration]
     ret = pci_generic_config_write(bus, devfn, where, size, val);
           ^~~~~~~~~~~~~~~~~~~~~~~~
   drivers/pci//host/pcie-tango.c: At top level:
   drivers/pci//host/pcie-tango.c:256:3: error: field name not in record or union initializer
      .map_bus = pci_ecam_map_bus,
      ^
   drivers/pci//host/pcie-tango.c:256:3: note: (near initialization for 'smp8759_ecam_ops.pci_ops')
   drivers/pci//host/pcie-tango.c:257:3: error: field name not in record or union initializer
      .read  = smp8759_config_read,
      ^
   drivers/pci//host/pcie-tango.c:257:3: note: (near initialization for 'smp8759_ecam_ops.pci_ops')
   drivers/pci//host/pcie-tango.c:258:3: error: field name not in record or union initializer
      .write  = smp8759_config_write,
      ^
   drivers/pci//host/pcie-tango.c:258:3: note: (near initialization for 'smp8759_ecam_ops.pci_ops')
   drivers/pci//host/pcie-tango.c: In function 'tango_fixup_class':
   drivers/pci//host/pcie-tango.c:329:5: error: dereferencing pointer to incomplete type 'struct pci_dev'
     dev->class = PCI_CLASS_BRIDGE_PCI << 8;
        ^~
   drivers/pci//host/pcie-tango.c:329:15: error: 'PCI_CLASS_BRIDGE_PCI' undeclared (first use in this function)
     dev->class = PCI_CLASS_BRIDGE_PCI << 8;
                  ^~~~~~~~~~~~~~~~~~~~
   drivers/pci//host/pcie-tango.c: At top level:
   drivers/pci//host/pcie-tango.c:320:22: error: expected declaration specifiers or '...' before numeric constant
    #define VENDOR_SIGMA 0x1105
                         ^
   drivers/pci//host/pcie-tango.c:331:25: note: in expansion of macro 'VENDOR_SIGMA'
    DECLARE_PCI_FIXUP_EARLY(VENDOR_SIGMA, PCI_ANY_ID, tango_fixup_class);
                            ^~~~~~~~~~~~
   In file included from include/linux/of.h:22:0,
                    from include/linux/irqdomain.h:34,
                    from drivers/pci//host/pcie-tango.c:3:
>> include/linux/mod_devicetable.h:16:20: error: expected declaration specifiers or '...' before '(' token
    #define PCI_ANY_ID (~0)
                       ^
   drivers/pci//host/pcie-tango.c:331:39: note: in expansion of macro 'PCI_ANY_ID'
    DECLARE_PCI_FIXUP_EARLY(VENDOR_SIGMA, PCI_ANY_ID, tango_fixup_class);
                                          ^~~~~~~~~~
   drivers/pci//host/pcie-tango.c:331:51: error: expected declaration specifiers or '...' before 'tango_fixup_class'
    DECLARE_PCI_FIXUP_EARLY(VENDOR_SIGMA, PCI_ANY_ID, tango_fixup_class);
                                                      ^~~~~~~~~~~~~~~~~
   drivers/pci//host/pcie-tango.c: In function 'tango_fixup_bar':
   drivers/pci//host/pcie-tango.c:342:9: error: implicit declaration of function 'pci_write_config_dword' [-Werror=implicit-function-declaration]
            pci_write_config_dword(dev, PCI_BASE_ADDRESS_0, 0x80000000);
            ^~~~~~~~~~~~~~~~~~~~~~
   drivers/pci//host/pcie-tango.c:342:37: error: 'PCI_BASE_ADDRESS_0' undeclared (first use in this function)
            pci_write_config_dword(dev, PCI_BASE_ADDRESS_0, 0x80000000);
                                        ^~~~~~~~~~~~~~~~~~
   drivers/pci//host/pcie-tango.c: At top level:
   drivers/pci//host/pcie-tango.c:320:22: error: expected declaration specifiers or '...' before numeric constant
    #define VENDOR_SIGMA 0x1105
                         ^
   drivers/pci//host/pcie-tango.c:344:25: note: in expansion of macro 'VENDOR_SIGMA'
    DECLARE_PCI_FIXUP_FINAL(VENDOR_SIGMA, PCI_ANY_ID, tango_fixup_bar);
                            ^~~~~~~~~~~~
   In file included from include/linux/of.h:22:0,
                    from include/linux/irqdomain.h:34,
                    from drivers/pci//host/pcie-tango.c:3:
>> include/linux/mod_devicetable.h:16:20: error: expected declaration specifiers or '...' before '(' token
    #define PCI_ANY_ID (~0)
                       ^
   drivers/pci//host/pcie-tango.c:344:39: note: in expansion of macro 'PCI_ANY_ID'
    DECLARE_PCI_FIXUP_FINAL(VENDOR_SIGMA, PCI_ANY_ID, tango_fixup_bar);
                                          ^~~~~~~~~~
   drivers/pci//host/pcie-tango.c:344:51: error: expected declaration specifiers or '...' before 'tango_fixup_bar'
    DECLARE_PCI_FIXUP_FINAL(VENDOR_SIGMA, PCI_ANY_ID, tango_fixup_bar);
                                                      ^~~~~~~~~~~~~~~
   drivers/pci//host/pcie-tango.c:340:13: warning: 'tango_fixup_bar' defined but not used [-Wunused-function]
    static void tango_fixup_bar(struct pci_dev *dev)
                ^~~~~~~~~~~~~~~
   drivers/pci//host/pcie-tango.c:327:13: warning: 'tango_fixup_class' defined but not used [-Wunused-function]
    static void tango_fixup_class(struct pci_dev *dev)
                ^~~~~~~~~~~~~~~~~
   cc1: some warnings being treated as errors

vim +/pci_ops +29 include/linux/pci-ecam.h

35ff9477 drivers/pci/ecam.h       Jayachandran C 2016-05-10  23   * struct to hold pci ops and bus shift of the config window
35ff9477 drivers/pci/ecam.h       Jayachandran C 2016-05-10  24   * for a PCI controller.
35ff9477 drivers/pci/ecam.h       Jayachandran C 2016-05-10  25   */
35ff9477 drivers/pci/ecam.h       Jayachandran C 2016-05-10  26  struct pci_config_window;
35ff9477 drivers/pci/ecam.h       Jayachandran C 2016-05-10  27  struct pci_ecam_ops {
35ff9477 drivers/pci/ecam.h       Jayachandran C 2016-05-10  28  	unsigned int			bus_shift;
35ff9477 drivers/pci/ecam.h       Jayachandran C 2016-05-10 @29  	struct pci_ops			pci_ops;
5c3d14f7 include/linux/pci-ecam.h Jayachandran C 2016-06-10  30  	int				(*init)(struct pci_config_window *);
35ff9477 drivers/pci/ecam.h       Jayachandran C 2016-05-10  31  };
35ff9477 drivers/pci/ecam.h       Jayachandran C 2016-05-10  32  
35ff9477 drivers/pci/ecam.h       Jayachandran C 2016-05-10  33  /*
35ff9477 drivers/pci/ecam.h       Jayachandran C 2016-05-10  34   * struct to hold the mappings of a config space window. This
35ff9477 drivers/pci/ecam.h       Jayachandran C 2016-05-10  35   * is expected to be used as sysdata for PCI controllers that
35ff9477 drivers/pci/ecam.h       Jayachandran C 2016-05-10  36   * use ECAM.
35ff9477 drivers/pci/ecam.h       Jayachandran C 2016-05-10  37   */
35ff9477 drivers/pci/ecam.h       Jayachandran C 2016-05-10  38  struct pci_config_window {
35ff9477 drivers/pci/ecam.h       Jayachandran C 2016-05-10  39  	struct resource			res;
35ff9477 drivers/pci/ecam.h       Jayachandran C 2016-05-10  40  	struct resource			busr;
35ff9477 drivers/pci/ecam.h       Jayachandran C 2016-05-10  41  	void				*priv;
35ff9477 drivers/pci/ecam.h       Jayachandran C 2016-05-10  42  	struct pci_ecam_ops		*ops;
35ff9477 drivers/pci/ecam.h       Jayachandran C 2016-05-10  43  	union {
35ff9477 drivers/pci/ecam.h       Jayachandran C 2016-05-10  44  		void __iomem		*win;	/* 64-bit single mapping */
35ff9477 drivers/pci/ecam.h       Jayachandran C 2016-05-10  45  		void __iomem		**winp; /* 32-bit per-bus mapping */
35ff9477 drivers/pci/ecam.h       Jayachandran C 2016-05-10  46  	};
5c3d14f7 include/linux/pci-ecam.h Jayachandran C 2016-06-10  47  	struct device			*parent;/* ECAM res was from this dev */
35ff9477 drivers/pci/ecam.h       Jayachandran C 2016-05-10  48  };
35ff9477 drivers/pci/ecam.h       Jayachandran C 2016-05-10  49  
35ff9477 drivers/pci/ecam.h       Jayachandran C 2016-05-10  50  /* create and free pci_config_window */
35ff9477 drivers/pci/ecam.h       Jayachandran C 2016-05-10  51  struct pci_config_window *pci_ecam_create(struct device *dev,
35ff9477 drivers/pci/ecam.h       Jayachandran C 2016-05-10  52  		struct resource *cfgres, struct resource *busr,
35ff9477 drivers/pci/ecam.h       Jayachandran C 2016-05-10  53  		struct pci_ecam_ops *ops);
35ff9477 drivers/pci/ecam.h       Jayachandran C 2016-05-10  54  void pci_ecam_free(struct pci_config_window *cfg);
35ff9477 drivers/pci/ecam.h       Jayachandran C 2016-05-10  55  
35ff9477 drivers/pci/ecam.h       Jayachandran C 2016-05-10  56  /* map_bus when ->sysdata is an instance of pci_config_window */
35ff9477 drivers/pci/ecam.h       Jayachandran C 2016-05-10 @57  void __iomem *pci_ecam_map_bus(struct pci_bus *bus, unsigned int devfn,
35ff9477 drivers/pci/ecam.h       Jayachandran C 2016-05-10  58  			       int where);
35ff9477 drivers/pci/ecam.h       Jayachandran C 2016-05-10  59  /* default ECAM ops */
35ff9477 drivers/pci/ecam.h       Jayachandran C 2016-05-10  60  extern struct pci_ecam_ops pci_generic_ecam_ops;

:::::: The code at line 29 was first introduced by commit
:::::: 35ff9477d880986441981010585399c1d7201fee PCI: Provide common functions for ECAM mapping

:::::: TO: Jayachandran C <jchandra@broadcom.com>
:::::: CC: Bjorn Helgaas <bhelgaas@google.com>

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 59492 bytes --]

Re: [PATCH v3 2/2] PCI: Add tango PCIe host bridge support

[Rob Herring]

On Wed, Mar 29, 2017 at 01:34:45PM +0200, Marc Gonzalez wrote:
> This driver is used to work around HW bugs in the controller.
> 
> Note: the controller does NOT support the following features.
> 
>   Legacy PCI interrupts
>   IO space
> 
> Signed-off-by: Marc Gonzalez <marc_gonzalez@sigmadesigns.com>
> ---
>  Documentation/devicetree/bindings/pci/tango-pcie.txt |  33 +++++++++

Acked-by: Rob Herring <robh@kernel.org> 

>  drivers/pci/host/Kconfig                             |   7 ++
>  drivers/pci/host/Makefile                            |   1 +
>  drivers/pci/host/pcie-tango.c                        | 150 +++++++++++++++++++++++++++++++++++++++
>  4 files changed, 191 insertions(+)