[PATCH v2 0/3] x86/pci/intel-mid-pci: fix to get eMMC detected

[PATCH v2 0/3] x86/pci/intel-mid-pci: fix to get eMMC detected

[Andy Shevchenko]

On Intel Edison we have an interesting implementation of x86 platform without
legacy PIC and with specific PCI. There are devices which are not using
interrupt by some reasons, but have them as IRQ0 in the PCI configuration.
Suprisingly the first eMMC host controller is the actual user for IRQ0. Since
we have serial driver implemented that enumerates unused serial IP (one of
four) which has IRQ0 assigned we, in case it gets it first by
pci_enable_device(), lost a possibility to probe eMMC. 

So, this series provides a quirk (patch 1), small fix of error code
(patch 2), and sparse warning fix (patch 3).

Changelog v2:
 - rearrange patches 1 and 2 to provide fix first with Fixes: tag
 - append patch 3
 - rebase on top of recent linux-next

Andy Shevchenko (3):
  x86/pci/intel_mid_pci: work around for IRQ0 assignment
  x86/pci/intel_mid_pci: propagate actual return code
  x86/pci/intel_mid_pci: fix a sparse warning

 arch/x86/pci/intel_mid_pci.c | 27 ++++++++++++++++++++++++---
 1 file changed, 24 insertions(+), 3 deletions(-)

-- 
2.1.4

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

[Andy Shevchenko]

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(+)

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

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

[Thomas Gleixner]

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

[PATCH v2 2/3] x86/pci/intel_mid_pci: propagate actual return code

[Andy Shevchenko]

mp_map_gsi_to_irq() returns different codes if it fails.
intel_mid_pci_irq_enable() hides this under -EBUSY. The patch replaces it by
what is actually returned.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 arch/x86/pci/intel_mid_pci.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/arch/x86/pci/intel_mid_pci.c b/arch/x86/pci/intel_mid_pci.c
index 5d7f4afe..4739834 100644
--- a/arch/x86/pci/intel_mid_pci.c
+++ b/arch/x86/pci/intel_mid_pci.c
@@ -212,6 +212,7 @@ 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)
 		return 0;
@@ -232,6 +233,7 @@ static int intel_mid_pci_irq_enable(struct pci_dev *dev)
 		}
 	}
 
+	/* Set IRQ polarity */
 	if (intel_mid_identify_cpu() == INTEL_MID_CPU_CHIP_TANGIER)
 		polarity = 0; /* active high */
 	else
@@ -242,8 +244,9 @@ static int intel_mid_pci_irq_enable(struct pci_dev *dev)
 	 * MRST only have IOAPIC, the PCI irq lines are 1:1 mapped to
 	 * IOAPIC RTE entries, so we just enable RTE for the device.
 	 */
-	if (mp_map_gsi_to_irq(dev->irq, IOAPIC_MAP_ALLOC, &info) < 0)
-		return -EBUSY;
+	ret = mp_map_gsi_to_irq(dev->irq, IOAPIC_MAP_ALLOC, &info);
+	if (ret < 0)
+		return ret;
 
 	dev->irq_managed = 1;
 
-- 
2.1.4

[PATCH v2 3/3] x86/pci/intel_mid_pci: fix a sparse warning

[Andy Shevchenko]

This fixes the following sparse warning.
arch/x86/pci/intel_mid_pci.c:265:16: warning: symbol 'intel_mid_pci_ops' was not declared. Should it be static?

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 arch/x86/pci/intel_mid_pci.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/x86/pci/intel_mid_pci.c b/arch/x86/pci/intel_mid_pci.c
index 4739834..65f2d9f 100644
--- a/arch/x86/pci/intel_mid_pci.c
+++ b/arch/x86/pci/intel_mid_pci.c
@@ -262,7 +262,7 @@ static void intel_mid_pci_irq_disable(struct pci_dev *dev)
 	}
 }
 
-struct pci_ops intel_mid_pci_ops = {
+static struct pci_ops intel_mid_pci_ops = {
 	.read = pci_read,
 	.write = pci_write,
 };
-- 
2.1.4