On Wed, 8 Jul 2015, Andy Shevchenko wrote:
> A few devices on Intel Edison board (Intel Tangier) has IRQ0 as an IRQ line in
> the PCI configuration. The actual one which is using that is a first eMMC host
> controller.
You fail to explain that the other devices have a bogus PCI configuration.
> In case we compile sdhci-pci as a module and leave serial driver built-in,
> first serial device not in use and has IRQ0 assigned as well, the latter takes
> the interrupt allocation.
We are really not interested in the details of whats compiled in or
not and which other device is acquiring the interrupt. What matters
is: It's an init ordering problem.
> The result of such behaviour is impossibility to
> allocate the interrupt by sdhci-pci driver.
>
> This patch introduces a quirk inside intel_mid_pci_irq_enable() to avoid
> described behaviour.
That's pretty useless. You tell the reader that you add a quirk, which
is hardly interesting because the subject line already talks about a
workaround. You fail to tell WHAT the quirk is doing.
Aside of that, starting a sentence in a changelog with "This patch" is
silly. We already know that THIS is a patch.
Let me rephrase the whole thing:
"On Intel Tangier the MMC host controller is wired up to irq 0. But
several other devices have irq 0 associated as well due to a bogus
PCI configuration.
The first initialized driver will acquire irq 0 and make it
unavailable for other devices. If the sdhci driver is not the first
one it will fail to acquire the interrupt and therefor be non
functional.
Add a quirk to the pci irq enable function which denies irq 0 to
anything else than the MMC host controller driver on Tangier
platforms."
Can you see the difference?
> +#define PCI_DEVICE_ID_INTEL_MRFL_MMC 0x1190
> +
Please add defines at the top of the file, not just randomly in the
middle of the code.
> static int intel_mid_pci_irq_enable(struct pci_dev *dev)
> {
> struct irq_alloc_info info;
> int polarity;
> int ret;
>
> - if (dev->irq_managed && dev->irq > 0)
> + if (dev->irq_managed && dev->irq >= 0)
> return 0;
What's the point here? Can dev->irq_managed be set and dev->irq be < 0?
> + /* Special treatment for IRQ0 */
> + if (dev->irq == 0) {
> + switch (intel_mid_identify_cpu()) {
> + case INTEL_MID_CPU_CHIP_TANGIER:
> + /*
> + * TNG has IRQ0 assigned to eMMC controller. This makes
> + * it happy to get an interrupt.
It's nice that you want to make the eMMC controller happy, but I doubt
that the silicon actually cares.
Please add a proper comment explaining the issue at hand.
> + */
> + if (dev->device != PCI_DEVICE_ID_INTEL_MRFL_MMC)
> + return -EBUSY;
> + break;
> + default:
> + break;
> + }
> + }
> +
> /* Set IRQ polarity */
> if (intel_mid_identify_cpu() == INTEL_MID_CPU_CHIP_TANGIER)
> polarity = 0; /* active high */
So now we have:
if (dev->irq == 0) {
switch(intel_mid_identify_cpu()) {
case INTEL_MID_CPU_CHIP_TANGIER:
....
}
and right after that:
if (intel_mid_identify_cpu() == INTEL_MID_CPU_CHIP_TANGIER)
....
That's just silly. Whats wrong with:
switch (intel_mid_identify_cpu()) {
case INTEL_MID_CPU_CHIP_TANGIER:
polarity = 0;
if (dev->irq == 0) {
....
}
break;
default:
polarity = 1;
}
Hmm?
tglx
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
