Skip to contentSkip to conversation

gpu/radeon: Move 64-bit MSI quirk from arch to driver

PATCH In review Benjamin Herrenschmidt 3 participants 6 messages Oct 2, 2014

Subsystems (4)
lore
Share & tools
lore
#0 lore raw

PATCH v2 2/4 gpu/radeon: Move 64-bit MSI quirk from arch to driver

A number of radeon cards have a HW limitation causing them to be unable to generate the full 64-bit of address bits for MSIs. This breaks MSIs on some platforms such as POWER machines.

We used to have a powerpc specific quirk to address that on a single card, but this doesn't scale very well, this is better put under control of the drivers who know precisely what a given HW revision can do.

This moves the setting of the quirk flag to the radeon driver

---

v2: This is just adjusted to the new flag name

 arch/powerpc/kernel/pci_64.c            |  1 -
 drivers/gpu/drm/radeon/radeon_irq_kms.c | 10 ++++++++++
 2 files changed, 10 insertions(+), 1 deletion(-)
arch/powerpc/kernel/pci_64.c +0 -1
index d41a831..5330f6d 100644
@@ -271,5 +271,4 @@ static void quirk_radeon_32bit_msi(struct pci_dev *dev)
 {
 	dev->no_64bit_msi = true;
 }
-DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_ATI, 0x68f2, quirk_radeon_32bit_msi);
 DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_ATI, 0xaa68, quirk_radeon_32bit_msi);
drivers/gpu/drm/radeon/radeon_irq_kms.c +10 -0
index 16807af..e760671 100644
@@ -202,6 +202,16 @@ static bool radeon_msi_ok(struct radeon_device *rdev)
 	if (rdev->flags & RADEON_IS_AGP)
 		return false;
 
+	/*
+	 * Older chips have a HW limitation, they can only generate 40 bits
+	 * of address for "64-bit" MSIs which breaks on some platforms, notably
+	 * IBM POWER servers, so we limit them
+	 */
+	if (rdev->family < CHIP_BONAIRE) {
+		dev_info(rdev->dev, "radeon: MSI limited to 32-bit\n");
+		rdev->pdev->no_64bit_msi = true;
+	}
+
 	/* force MSI on */
 	if (radeon_msi == 1)
 		return true;
#1 lore raw

Hi Ben,

Quoted 5 lines · On Thu, 02 Oct 2014 10:34:22 +1000 Benjamin Herrenschmidt…
drivers/gpu/drm/radeon/radeon_irq_kms.c +0 -0
@@ -202,6 +202,16 @@ static bool radeon_msi_ok(struct radeon_device *rdev)
 	if (rdev->flags & RADEON_IS_AGP)

drivers/gpu/drm/radeon/radeon_irq_kms.c static bool radeon_msi_ok(struct radeon_device *rdev) · line 212

Quoted 10 lines
drivers/gpu/drm/radeon/radeon_irq_kms.c +8 -0
 		return false;
 
+	/*
+	 * Older chips have a HW limitation, they can only generate 40 bits
+	 * of address for "64-bit" MSIs which breaks on some platforms, notably
+	 * IBM POWER servers, so we limit them
+	 */
+	if (rdev->family < CHIP_BONAIRE) {
+		dev_info(rdev->dev, "radeon: MSI limited to 32-bit\n");
+		rdev->pdev->no_64bit_msi = true;

Again, no_64bit_msi is not a bool ...

Signature · 2 lines
Cheers,
Stephen Rothwell                    [email protected]
maintainer
#2 lore raw
In reply to Benjamin Herrenschmidt
Quoted 10 lines · Benjamin Herrenschmidt

A number of radeon cards have a HW limitation causing them to be unable to generate the full 64-bit of address bits for MSIs. This breaks MSIs on some platforms such as POWER machines.

We used to have a powerpc specific quirk to address that on a single card, but this doesn't scale very well, this is better put under control of the drivers who know precisely what a given HW revision can do.

Quoted 6 lines

This moves the setting of the quirk flag to the radeon driver

---

v2: This is just adjusted to the new flag name

I'm sorta confused because I got two "v2 2/4" emails a minute or so apart. I assume they're the same.

Quoted 10 lines
 arch/powerpc/kernel/pci_64.c            |  1 -
 drivers/gpu/drm/radeon/radeon_irq_kms.c | 10 ++++++++++
 2 files changed, 10 insertions(+), 1 deletion(-)
arch/powerpc/kernel/pci_64.c +0 -0
index d41a831..5330f6d 100644

arch/powerpc/kernel/pci_64.c static void quirk_radeon_32bit_msi(struct pci_dev *dev) · line 274

Quoted 5 lines
arch/powerpc/kernel/pci_64.c +0 -1
@@ -271,5 +271,4 @@ static void quirk_radeon_32bit_msi(struct pci_dev *dev)
 {
 	dev->no_64bit_msi = true;
 }
-DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_ATI, 0x68f2, quirk_radeon_32bit_msi);
 DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_ATI, 0xaa68, quirk_radeon_32bit_msi);

Why do we keep the 0xaa68 quirk? Shouldn't that be made generic, too?

Quoted 24 lines
drivers/gpu/drm/radeon/radeon_irq_kms.c +10 -0
index 16807af..e760671 100644
@@ -202,6 +202,16 @@ static bool radeon_msi_ok(struct radeon_device *rdev)
 	if (rdev->flags & RADEON_IS_AGP)
 		return false;
 
+	/*
+	 * Older chips have a HW limitation, they can only generate 40 bits
+	 * of address for "64-bit" MSIs which breaks on some platforms, notably
+	 * IBM POWER servers, so we limit them
+	 */
+	if (rdev->family < CHIP_BONAIRE) {
+		dev_info(rdev->dev, "radeon: MSI limited to 32-bit\n");
+		rdev->pdev->no_64bit_msi = true;
+	}
+
 	/* force MSI on */
 	if (radeon_msi == 1)
 		return true;
#3 lore raw
Quoted 13 lines · Bjorn Helgaas
Quoted 12 lines · Benjamin Herrenschmidt

A number of radeon cards have a HW limitation causing them to be unable to generate the full 64-bit of address bits for MSIs. This breaks MSIs on some platforms such as POWER machines.

We used to have a powerpc specific quirk to address that on a single card, but this doesn't scale very well, this is better put under control of the drivers who know precisely what a given HW revision can do.

This moves the setting of the quirk flag to the radeon driver

Quoted 7 lines
Quoted 4 lines

---

v2: This is just adjusted to the new flag name

I'm sorta confused because I got two "v2 2/4" emails a minute or so apart. I assume they're the same.

Yes, evo blew up while sending the series the first time around :(

Quoted 42 lines
Quoted 10 lines
 arch/powerpc/kernel/pci_64.c            |  1 -
 drivers/gpu/drm/radeon/radeon_irq_kms.c | 10 ++++++++++
 2 files changed, 10 insertions(+), 1 deletion(-)
arch/powerpc/kernel/pci_64.c +0 -0
index d41a831..5330f6d 100644

arch/powerpc/kernel/pci_64.c static void quirk_radeon_32bit_msi(struct pci_dev *dev) · line 274

Quoted 5 lines
arch/powerpc/kernel/pci_64.c +0 -1
@@ -271,5 +271,4 @@ static void quirk_radeon_32bit_msi(struct pci_dev *dev)
 {
 	dev->no_64bit_msi = true;
 }
-DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_ATI, 0x68f2, quirk_radeon_32bit_msi);
 DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_ATI, 0xaa68, quirk_radeon_32bit_msi);

Why do we keep the 0xaa68 quirk? Shouldn't that be made generic, too?

Quoted 24 lines
drivers/gpu/drm/radeon/radeon_irq_kms.c +10 -0
index 16807af..e760671 100644
@@ -202,6 +202,16 @@ static bool radeon_msi_ok(struct radeon_device *rdev)
 	if (rdev->flags & RADEON_IS_AGP)
 		return false;
 
+	/*
+	 * Older chips have a HW limitation, they can only generate 40 bits
+	 * of address for "64-bit" MSIs which breaks on some platforms, notably
+	 * IBM POWER servers, so we limit them
+	 */
+	if (rdev->family < CHIP_BONAIRE) {
+		dev_info(rdev->dev, "radeon: MSI limited to 32-bit\n");
+		rdev->pdev->no_64bit_msi = true;
+	}
+
 	/* force MSI on */
 	if (radeon_msi == 1)
 		return true;
maintainer
#4 lore raw
Quoted 17 lines · Benjamin Herrenschmidt
Quoted 16 lines · Bjorn Helgaas
Quoted 15 lines · Benjamin Herrenschmidt

A number of radeon cards have a HW limitation causing them to be unable to generate the full 64-bit of address bits for MSIs. This breaks MSIs on some platforms such as POWER machines.

We used to have a powerpc specific quirk to address that on a single card, but this doesn't scale very well, this is better put under control of the drivers who know precisely what a given HW revision can do.

This moves the setting of the quirk flag to the radeon driver

---

Quoted 6 lines
Quoted 4 lines
Quoted 1 line

v2: This is just adjusted to the new flag name

I'm sorta confused because I got two "v2 2/4" emails a minute or so apart. I assume they're the same.

Yes, evo blew up while sending the series the first time around :(

No problem. My real question is below, but you probably missed it because of my email gripe :)

Quoted 43 lines
Quoted 41 lines
Quoted 9 lines
 arch/powerpc/kernel/pci_64.c            |  1 -
 drivers/gpu/drm/radeon/radeon_irq_kms.c | 10 ++++++++++
 2 files changed, 10 insertions(+), 1 deletion(-)
arch/powerpc/kernel/pci_64.c +0 -0
index d41a831..5330f6d 100644

arch/powerpc/kernel/pci_64.c static void quirk_radeon_32bit_msi(struct pci_dev *dev) · line 274

Quoted 5 lines
arch/powerpc/kernel/pci_64.c +0 -1
@@ -271,5 +271,4 @@ static void quirk_radeon_32bit_msi(struct pci_dev *dev)
 {
    dev->no_64bit_msi = true;
 }
-DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_ATI, 0x68f2, quirk_radeon_32bit_msi);
 DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_ATI, 0xaa68, quirk_radeon_32bit_msi);

Why do we keep the 0xaa68 quirk? Shouldn't that be made generic, too?

Quoted 24 lines
drivers/gpu/drm/radeon/radeon_irq_kms.c +10 -0
index 16807af..e760671 100644
@@ -202,6 +202,16 @@ static bool radeon_msi_ok(struct radeon_device *rdev)
    if (rdev->flags & RADEON_IS_AGP)
            return false;
 
+   /*
+    * Older chips have a HW limitation, they can only generate 40 bits
+    * of address for "64-bit" MSIs which breaks on some platforms, notably
+    * IBM POWER servers, so we limit them
+    */
+   if (rdev->family < CHIP_BONAIRE) {
+           dev_info(rdev->dev, "radeon: MSI limited to 32-bit\n");
+           rdev->pdev->no_64bit_msi = true;
+   }
+
    /* force MSI on */
    if (radeon_msi == 1)
            return true;
#5 lore raw
Quoted 21 lines · Bjorn Helgaas
Quoted 19 lines · Benjamin Herrenschmidt
Quoted 18 lines · Bjorn Helgaas
Quoted 16 lines · Benjamin Herrenschmidt

A number of radeon cards have a HW limitation causing them to be unable to generate the full 64-bit of address bits for MSIs. This breaks MSIs on some platforms such as POWER machines.

We used to have a powerpc specific quirk to address that on a single card, but this doesn't scale very well, this is better put under control of the drivers who know precisely what a given HW revision can do.

This moves the setting of the quirk flag to the radeon driver

---

v2: This is just adjusted to the new flag name

Quoted 7 lines
Quoted 4 lines
Quoted 2 lines

I'm sorta confused because I got two "v2 2/4" emails a minute or so apart. I assume they're the same.

Yes, evo blew up while sending the series the first time around :(

No problem. My real question is below, but you probably missed it because of my email gripe :)

Heh ok :-)

Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
 arch/powerpc/kernel/pci_64.c            |  1 -
 drivers/gpu/drm/radeon/radeon_irq_kms.c | 10 ++++++++++
 2 files changed, 10 insertions(+), 1 deletion(-)
Quoted 12 lines
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
arch/powerpc/kernel/pci_64.c +0 -0
index d41a831..5330f6d 100644
Quoted 8 lines
Quoted 8 lines

arch/powerpc/kernel/pci_64.c static void quirk_radeon_32bit_msi(struct pci_dev *dev) · line 274

Quoted 6 lines
arch/powerpc/kernel/pci_64.c +0 -1
@@ -271,5 +271,4 @@ static void quirk_radeon_32bit_msi(struct pci_dev *dev)
 {
    dev->no_64bit_msi = true;
 }
-DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_ATI, 0x68f2, quirk_radeon_32bit_msi);
 DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_ATI, 0xaa68, quirk_radeon_32bit_msi);

Why do we keep the 0xaa68 quirk? Shouldn't that be made generic, too?

aa68 is the audio part, it's removed by the audio driver patch.

But I can break things down into smaller bits as you suggested. I'll try to get that sorted later today.

Cheers,
Ben.

Quoted 26 lines
Quoted 26 lines
Quoted 24 lines
Quoted 24 lines
drivers/gpu/drm/radeon/radeon_irq_kms.c +10 -0
index 16807af..e760671 100644
@@ -202,6 +202,16 @@ static bool radeon_msi_ok(struct radeon_device *rdev)
    if (rdev->flags & RADEON_IS_AGP)
            return false;
 
+   /*
+    * Older chips have a HW limitation, they can only generate 40 bits
+    * of address for "64-bit" MSIs which breaks on some platforms, notably
+    * IBM POWER servers, so we limit them
+    */
+   if (rdev->family < CHIP_BONAIRE) {
+           dev_info(rdev->dev, "radeon: MSI limited to 32-bit\n");
+           rdev->pdev->no_64bit_msi = true;
+   }
+
    /* force MSI on */
    if (radeon_msi == 1)
            return true;