diff mbox

[v2,1/3] x86/pci/intel_mid_pci: work around for IRQ0 assignment

Message ID 1436354059-130135-2-git-send-email-andriy.shevchenko@linux.intel.com (mailing list archive)
State New, archived
Delegated to: Bjorn Helgaas
Headers show

Commit Message

Andy Shevchenko July 8, 2015, 11:14 a.m. UTC
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(+)

Comments

Thomas Gleixner July 8, 2015, 12:55 p.m. UTC | #1
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 mbox

Patch

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