[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

[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...

[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.

[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.

[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 at 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

[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@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 at 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);
> 

[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.

[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.
> 

[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

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

[kbuild test robot]

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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: .config.gz
Type: application/gzip
Size: 59492 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20170330/adbb5d8f/attachment-0001.gz>

[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(+)

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

[Mason]

On 29/03/2017 13:34, Marc Gonzalez wrote:

> +	/*
> +	 * 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 */
> +	}

AFAICT, the relevant caller is:

bool pci_bus_read_dev_vendor_id(struct pci_bus *bus, int devfn, u32 *l,
				int crs_timeout)

	if (pci_bus_read_config_dword(bus, devfn, PCI_VENDOR_ID, l))
		return false;

Therefore, I believe updating *val is unnecessary.
What matters is returning an error when appropriate.
PCIBIOS_FUNC_NOT_SUPPORTED fits my purpose.


> +
> +	/*
> +	 * 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? */
> +	}
AFAICT, the relevant caller is:

int __pci_read_base(struct pci_dev *dev, enum pci_bar_type type,
		    struct resource *res, unsigned int pos)

	u32 l, sz, mask;

	pci_read_config_dword(dev, pos, &l);
	pci_write_config_dword(dev, pos, l | mask);
	pci_read_config_dword(dev, pos, &sz);
	pci_write_config_dword(dev, pos, l);


Several things are note-worthy:

1) __pci_read_base() ignores the config accessors return value.
Of the existing PCIBIOS errors, none seem to be a good fit for
my use-case (hiding a non-standard BAR).

Tangent:

Maybe I should set dev->non_compliant_bars = true; instead
of messing around in the accessor...

I would likely set the flag in a PCI_FIXUP_EARLY function.
/* Early fixups, before probing the BARs */


1b) Perhaps a generic error could be added to the PCIBIOS_*
error family, to signal that the requested operation was not
completed, and *val remains unchanged.
=> Maybe PCIBIOS_FAILURE or PCIBIOS_NOP ?


2) Some drivers are not aware that *val needs to be updated
for BAR accesses.

e.g. drivers/pci/host/pcie-altera.c

if altera_pcie_hide_rc_bar() is true, 'l' and 'sz' are not updated,
and therefore contain garbage (uninitialized stack variables).

I think we should apply the following trivial patch.

--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -175,7 +175,7 @@ static inline unsigned long decode_bar(struct pci_dev *dev, u32 bar)
 int __pci_read_base(struct pci_dev *dev, enum pci_bar_type type,
                    struct resource *res, unsigned int pos)
 {
-       u32 l, sz, mask;
+       u32 l = 0, sz = 0, mask;
        u64 l64, sz64, mask64;
        u16 orig_cmd;
        struct pci_bus_region region, inverted_region;


Regards.

[PATCH v3 0/2] Tango PCIe controller support

[Mason]

On 29/03/2017 13:11, Marc Gonzalez wrote:

>   PCI: Add tango MSI controller support
>   PCI: Add tango PCIe host bridge support

I've run into an issue.

If I boot the system with earlyprintk enabled (as I've
been doing throughout my dev), things work as expected.

But if I boot with earlyprintk disabled, then the system
does not "see" the PCIe board, because reading the vendor
ID returns 0xffffffff.

What we think is happening, is that when earlyprintk is
disabled, the system proceeds much faster through the
various inits, and the PCIe init happens when PCIe
link training has not completed yet.

If that is the case, then it seems I would need to check
the link state in my probe function.

Or would there be some other solution I haven't thought
about?

Regards.

[PATCH v3 0/2] Tango PCIe controller support

[Mason]

On 30/03/2017 22:56, Mason wrote:

> I've run into an issue.
> 
> If I boot the system with earlyprintk enabled (as I've
> been doing throughout my dev), things work as expected.
> 
> But if I boot with earlyprintk disabled, then the system
> does not "see" the PCIe board, because reading the vendor
> ID returns 0xffffffff.
> 
> What we think is happening, is that when earlyprintk is
> disabled, the system proceeds much faster through the
> various inits, and the PCIe init happens when PCIe
> link training has not completed yet.
> 
> If that is the case, then it seems I would need to check
> the link state in my probe function.

I determined empirically that link training takes around
10-15 ms. Though I suppose this might depend on the
specific PCIe board? (I'm only considering x1 link.)

So I added an msleep(20); in the probe function, and in
the config_read callback, I check the link status on the
first read to the device.

Should I msleep(40) to be safe?

Regards.