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