| Message ID | 1436354059-130135-2-git-send-email-andriy.shevchenko@linux.intel.com |
|---|---|
| State | Changes Requested |
| Headers | show |
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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/arch/x86/pci/intel_mid_pci.c b/arch/x86/pci/intel_mid_pci.c index 2706230..5d7f4afe 100644 --- a/arch/x86/pci/intel_mid_pci.c +++ b/arch/x86/pci/intel_mid_pci.c @@ -206,6 +206,8 @@ static int pci_write(struct pci_bus *bus, unsigned int devfn, int where, where, size, value); } +#define PCI_DEVICE_ID_INTEL_MRFL_MMC 0x1190 + static int intel_mid_pci_irq_enable(struct pci_dev *dev) { struct irq_alloc_info info; @@ -214,6 +216,22 @@ static int intel_mid_pci_irq_enable(struct pci_dev *dev) if (dev->irq_managed && dev->irq > 0) return 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. + */ + if (dev->device != PCI_DEVICE_ID_INTEL_MRFL_MMC) + return -EBUSY; + break; + default: + break; + } + } + if (intel_mid_identify_cpu() == INTEL_MID_CPU_CHIP_TANGIER) polarity = 0; /* active high */ else
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. 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. 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. Fixes: 90b9aacf912a (serial: 8250_pci: add Intel Tangier support) Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> --- arch/x86/pci/intel_mid_pci.c | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+)