On 29/03/17 13:53, Mason wrote:

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

quoted

On 29/03/17 12:34, Marc Gonzalez wrote:

quoted

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

quoted

quoted

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

quoted

quoted

+#define VENDOR_SIGMA	0x1105

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

Doh! Very likely. Thanks.

Regards.