[PATCH v2] PCI: Support hibmc VGA cards behind a misbehaving HiSilicon bridge

[PATCH v2] PCI: Support hibmc VGA cards behind a misbehaving HiSilicon bridge

[Daniel Axtens]

The HiSilicon D05 board has some PCI bridges (PCI ID 19e5:1610) that
are not spec-compliant: the VGA Enable bit is set to 0 in hardware
and writes do not change it.

This stops VGA arbitrartion from marking a VGA card behind the bridge
as a boot device, and therefore breaks Xorg auto-configuration.

The hibmc VGA card (PCI ID 19e5:1711) is known to work when behind
these bridges.

Provide a quirk so that this combination of bridge and card is eligible
to be the default VGA card.

This fixes Xorg auto-detection.

Cc: Xinliang Liu <z.liuxinliang@hisilicon.com>
Cc: Rongrong Zou <zourongrong@gmail.com>
Signed-off-by: Daniel Axtens <dja@axtens.net>
---
 drivers/pci/quirks.c | 46 ++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 46 insertions(+)

diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
index 16e6cd86ad71..86d7c848f3e2 100644
--- a/drivers/pci/quirks.c
+++ b/drivers/pci/quirks.c
@@ -25,6 +25,7 @@
 #include <linux/sched.h>
 #include <linux/ktime.h>
 #include <linux/mm.h>
+#include <linux/vgaarb.h>
 #include <asm/dma.h>	/* isa_dma_bridge_buggy */
 #include "pci.h"
 
@@ -4664,3 +4665,48 @@ static void quirk_intel_no_flr(struct pci_dev *dev)
 }
 DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_INTEL, 0x1502, quirk_intel_no_flr);
 DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_INTEL, 0x1503, quirk_intel_no_flr);
+
+/*
+ * The hibmc card on a HiSilicon D05 board sits behind a non-compliant
+ * bridge. The bridge has the PCI_BRIDGE_CTL_VGA config bit fixed at 0
+ * in hardware. This prevents the vgaarb from marking a card behind it
+ * as boot VGA device.
+ *
+ * However, the hibmc card is known to still work, so if we have that
+ * card behind that particular bridge (19e5:1610), mark it as the
+ * default device if none has been detected.
+ */
+static void hibmc_fixup_vgaarb(struct pci_dev *pdev)
+{
+	struct pci_dev *bridge;
+	struct pci_bus *bus;
+	u16 config;
+
+	bus = pdev->bus;
+	bridge = bus->self;
+	if (!bridge)
+		return;
+
+	if (!pci_is_bridge(bridge))
+		return;
+
+	if (bridge->vendor != PCI_VENDOR_ID_HUAWEI ||
+	    bridge->device != 0x1610)
+		return;
+
+	pci_read_config_word(bridge, PCI_BRIDGE_CONTROL,
+			     &config);
+	if (config & PCI_BRIDGE_CTL_VGA) {
+		/*
+		 * Weirdly, this bridge *is* spec compliant, so bail
+		 * and let vgaarb do its job
+		 */
+		return;
+	}
+
+	if (vga_default_device())
+		return;
+
+	vga_set_default_device(pdev);
+}
+DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_HUAWEI, 0x1711, hibmc_fixup_vgaarb);
-- 
2.11.0

Re: [PATCH v2] PCI: Support hibmc VGA cards behind a misbehaving HiSilicon bridge

[Bjorn Helgaas]

Hi Daniel,

s/arbitrartion/arbitration/ below

On Tue, Jun 20, 2017 at 11:33:02AM +1000, Daniel Axtens wrote:
> The HiSilicon D05 board has some PCI bridges (PCI ID 19e5:1610) that
> are not spec-compliant: the VGA Enable bit is set to 0 in hardware
> and writes do not change it.
> 
> This stops VGA arbitrartion from marking a VGA card behind the bridge
> as a boot device, and therefore breaks Xorg auto-configuration.
> 
> The hibmc VGA card (PCI ID 19e5:1711) is known to work when behind
> these bridges.
> 
> Provide a quirk so that this combination of bridge and card is eligible
> to be the default VGA card.
> 
> This fixes Xorg auto-detection.

How exactly is this bridge broken?  Apparently the PCI_BRIDGE_CTL_VGA
is a read-only zero.  Does the bridge then *always* forward the
0xa0000-0xbffff mem range and the 0x3b0-0x3bb and 0x3c0-0x3df I/O
ranges?  (This is from the PCI-to-PCI Bridge Spec, r1.2, sec
3.2.5.18).

I don't know whether the hibmc VGA card requires those legacy VGA
resources, so the fact that the card works doesn't answer the
question -- the driver may be able to operate the card without those
legacy resources.

But if the system contains multiple VGA devices, we may not be able to
arbitrate between them correctly if this bit doesn't work.  For
example, if the bridge always forwards the legacy VGA resources, and a
second VGA devices behind a different bridge requires those resources,
we'll have a problem: a read may receive two responses.

The fact that this patch doesn't save any kind of "vga_broken" bit for
later use by the VGA arbiter makes me suspect that we're making the
device work in the current configuration, but things might break if we
add another VGA card.

> Cc: Xinliang Liu <z.liuxinliang@hisilicon.com>
> Cc: Rongrong Zou <zourongrong@gmail.com>
> Signed-off-by: Daniel Axtens <dja@axtens.net>
> ---
>  drivers/pci/quirks.c | 46 ++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 46 insertions(+)
> 
> diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
> index 16e6cd86ad71..86d7c848f3e2 100644
> --- a/drivers/pci/quirks.c
> +++ b/drivers/pci/quirks.c
> @@ -25,6 +25,7 @@
>  #include <linux/sched.h>
>  #include <linux/ktime.h>
>  #include <linux/mm.h>
> +#include <linux/vgaarb.h>
>  #include <asm/dma.h>	/* isa_dma_bridge_buggy */
>  #include "pci.h"
>  
> @@ -4664,3 +4665,48 @@ static void quirk_intel_no_flr(struct pci_dev *dev)
>  }
>  DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_INTEL, 0x1502, quirk_intel_no_flr);
>  DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_INTEL, 0x1503, quirk_intel_no_flr);
> +
> +/*
> + * The hibmc card on a HiSilicon D05 board sits behind a non-compliant
> + * bridge. The bridge has the PCI_BRIDGE_CTL_VGA config bit fixed at 0
> + * in hardware. This prevents the vgaarb from marking a card behind it
> + * as boot VGA device.
> + *
> + * However, the hibmc card is known to still work, so if we have that
> + * card behind that particular bridge (19e5:1610), mark it as the
> + * default device if none has been detected.
> + */
> +static void hibmc_fixup_vgaarb(struct pci_dev *pdev)
> +{
> +	struct pci_dev *bridge;
> +	struct pci_bus *bus;
> +	u16 config;
> +
> +	bus = pdev->bus;
> +	bridge = bus->self;
> +	if (!bridge)
> +		return;
> +
> +	if (!pci_is_bridge(bridge))
> +		return;
> +
> +	if (bridge->vendor != PCI_VENDOR_ID_HUAWEI ||
> +	    bridge->device != 0x1610)
> +		return;
> +
> +	pci_read_config_word(bridge, PCI_BRIDGE_CONTROL,
> +			     &config);
> +	if (config & PCI_BRIDGE_CTL_VGA) {
> +		/*
> +		 * Weirdly, this bridge *is* spec compliant, so bail
> +		 * and let vgaarb do its job
> +		 */
> +		return;
> +	}
> +
> +	if (vga_default_device())
> +		return;
> +
> +	vga_set_default_device(pdev);
> +}
> +DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_HUAWEI, 0x1711, hibmc_fixup_vgaarb);
> -- 
> 2.11.0
> 

Re: [PATCH v2] PCI: Support hibmc VGA cards behind a misbehaving HiSilicon bridge

[Daniel Axtens]

Hi Bjorn,

> How exactly is this bridge broken?  Apparently the PCI_BRIDGE_CTL_VGA
> is a read-only zero.  Does the bridge then *always* forward the
> 0xa0000-0xbffff mem range and the 0x3b0-0x3bb and 0x3c0-0x3df I/O
> ranges?  (This is from the PCI-to-PCI Bridge Spec, r1.2, sec
> 3.2.5.18).

Good question. I will forward this to the HiSilicon hardware engineers
and get their input. This may take a couple of days as there's a
language barrier.

> I don't know whether the hibmc VGA card requires those legacy VGA
> resources, so the fact that the card works doesn't answer the
> question -- the driver may be able to operate the card without those
> legacy resources.

My understanding is that the hibmc card does not require the legacy VGA
resources.

> But if the system contains multiple VGA devices, we may not be able to
> arbitrate between them correctly if this bit doesn't work.  For
> example, if the bridge always forwards the legacy VGA resources, and a
> second VGA devices behind a different bridge requires those resources,
> we'll have a problem: a read may receive two responses.

I see.

> The fact that this patch doesn't save any kind of "vga_broken" bit for
> later use by the VGA arbiter makes me suspect that we're making the
> device work in the current configuration, but things might break if we
> add another VGA card.

I'll have a think about that and get back to you when I hear back from
HiSilicon.

Regards,
Daniel

>> Cc: Xinliang Liu <z.liuxinliang@hisilicon.com>
>> Cc: Rongrong Zou <zourongrong@gmail.com>
>> Signed-off-by: Daniel Axtens <dja@axtens.net>
>> ---
>>  drivers/pci/quirks.c | 46 ++++++++++++++++++++++++++++++++++++++++++++++
>>  1 file changed, 46 insertions(+)
>> 
>> diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
>> index 16e6cd86ad71..86d7c848f3e2 100644
>> --- a/drivers/pci/quirks.c
>> +++ b/drivers/pci/quirks.c
>> @@ -25,6 +25,7 @@
>>  #include <linux/sched.h>
>>  #include <linux/ktime.h>
>>  #include <linux/mm.h>
>> +#include <linux/vgaarb.h>
>>  #include <asm/dma.h>	/* isa_dma_bridge_buggy */
>>  #include "pci.h"
>>  
>> @@ -4664,3 +4665,48 @@ static void quirk_intel_no_flr(struct pci_dev *dev)
>>  }
>>  DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_INTEL, 0x1502, quirk_intel_no_flr);
>>  DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_INTEL, 0x1503, quirk_intel_no_flr);
>> +
>> +/*
>> + * The hibmc card on a HiSilicon D05 board sits behind a non-compliant
>> + * bridge. The bridge has the PCI_BRIDGE_CTL_VGA config bit fixed at 0
>> + * in hardware. This prevents the vgaarb from marking a card behind it
>> + * as boot VGA device.
>> + *
>> + * However, the hibmc card is known to still work, so if we have that
>> + * card behind that particular bridge (19e5:1610), mark it as the
>> + * default device if none has been detected.
>> + */
>> +static void hibmc_fixup_vgaarb(struct pci_dev *pdev)
>> +{
>> +	struct pci_dev *bridge;
>> +	struct pci_bus *bus;
>> +	u16 config;
>> +
>> +	bus = pdev->bus;
>> +	bridge = bus->self;
>> +	if (!bridge)
>> +		return;
>> +
>> +	if (!pci_is_bridge(bridge))
>> +		return;
>> +
>> +	if (bridge->vendor != PCI_VENDOR_ID_HUAWEI ||
>> +	    bridge->device != 0x1610)
>> +		return;
>> +
>> +	pci_read_config_word(bridge, PCI_BRIDGE_CONTROL,
>> +			     &config);
>> +	if (config & PCI_BRIDGE_CTL_VGA) {
>> +		/*
>> +		 * Weirdly, this bridge *is* spec compliant, so bail
>> +		 * and let vgaarb do its job
>> +		 */
>> +		return;
>> +	}
>> +
>> +	if (vga_default_device())
>> +		return;
>> +
>> +	vga_set_default_device(pdev);
>> +}
>> +DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_HUAWEI, 0x1711, hibmc_fixup_vgaarb);
>> -- 
>> 2.11.0
>> 

Re: [PATCH v2] PCI: Support hibmc VGA cards behind a misbehaving HiSilicon bridge

[Daniel Axtens]

Hi Bjorn,

> How exactly is this bridge broken?  Apparently the PCI_BRIDGE_CTL_VGA
> is a read-only zero.  Does the bridge then *always* forward the
> 0xa0000-0xbffff mem range and the 0x3b0-0x3bb and 0x3c0-0x3df I/O
> ranges?  (This is from the PCI-to-PCI Bridge Spec, r1.2, sec
> 3.2.5.18).

So I asked the HiSilicon hardware folk, and they said "no". My
understanding is that the resources are never forwarded. The
justification I was given is that "this legacy mem range and IO are not
used in ARM". (The D05 is an arm64 system.)

> I don't know whether the hibmc VGA card requires those legacy VGA
> resources, so the fact that the card works doesn't answer the
> question -- the driver may be able to operate the card without those
> legacy resources.

I also asked the HiSilicon folk; and yes, the driver operates the card
without the legacy resources.

> But if the system contains multiple VGA devices, we may not be able to
> arbitrate between them correctly if this bit doesn't work.  For
> example, if the bridge always forwards the legacy VGA resources, and a
> second VGA devices behind a different bridge requires those resources,
> we'll have a problem: a read may receive two responses.
>
> The fact that this patch doesn't save any kind of "vga_broken" bit for
> later use by the VGA arbiter makes me suspect that we're making the
> device work in the current configuration, but things might break if we
> add another VGA card.

All the PCI ports on the system I have access to are behind these
bridges, so:

 - it looks like VGA cards will only work if their drivers support
   operating the card without legacy resources.

 - the vga default device should be set only when this quirk activates:
   normal VGA devices will fail the bridge capability test in
   vgaarb.c. If there are mutiple BMC cards then the check for an
   existing default device will ensure the default device is only set
   once.

Having said that, I am happy to write a patch to disable the arbiter if
you want - I'm just not sure quite how that would work.

Regards,
Daniel

>
>> Cc: Xinliang Liu <z.liuxinliang@hisilicon.com>
>> Cc: Rongrong Zou <zourongrong@gmail.com>
>> Signed-off-by: Daniel Axtens <dja@axtens.net>
>> ---
>>  drivers/pci/quirks.c | 46 ++++++++++++++++++++++++++++++++++++++++++++++
>>  1 file changed, 46 insertions(+)
>> 
>> diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
>> index 16e6cd86ad71..86d7c848f3e2 100644
>> --- a/drivers/pci/quirks.c
>> +++ b/drivers/pci/quirks.c
>> @@ -25,6 +25,7 @@
>>  #include <linux/sched.h>
>>  #include <linux/ktime.h>
>>  #include <linux/mm.h>
>> +#include <linux/vgaarb.h>
>>  #include <asm/dma.h>	/* isa_dma_bridge_buggy */
>>  #include "pci.h"
>>  
>> @@ -4664,3 +4665,48 @@ static void quirk_intel_no_flr(struct pci_dev *dev)
>>  }
>>  DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_INTEL, 0x1502, quirk_intel_no_flr);
>>  DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_INTEL, 0x1503, quirk_intel_no_flr);
>> +
>> +/*
>> + * The hibmc card on a HiSilicon D05 board sits behind a non-compliant
>> + * bridge. The bridge has the PCI_BRIDGE_CTL_VGA config bit fixed at 0
>> + * in hardware. This prevents the vgaarb from marking a card behind it
>> + * as boot VGA device.
>> + *
>> + * However, the hibmc card is known to still work, so if we have that
>> + * card behind that particular bridge (19e5:1610), mark it as the
>> + * default device if none has been detected.
>> + */
>> +static void hibmc_fixup_vgaarb(struct pci_dev *pdev)
>> +{
>> +	struct pci_dev *bridge;
>> +	struct pci_bus *bus;
>> +	u16 config;
>> +
>> +	bus = pdev->bus;
>> +	bridge = bus->self;
>> +	if (!bridge)
>> +		return;
>> +
>> +	if (!pci_is_bridge(bridge))
>> +		return;
>> +
>> +	if (bridge->vendor != PCI_VENDOR_ID_HUAWEI ||
>> +	    bridge->device != 0x1610)
>> +		return;
>> +
>> +	pci_read_config_word(bridge, PCI_BRIDGE_CONTROL,
>> +			     &config);
>> +	if (config & PCI_BRIDGE_CTL_VGA) {
>> +		/*
>> +		 * Weirdly, this bridge *is* spec compliant, so bail
>> +		 * and let vgaarb do its job
>> +		 */
>> +		return;
>> +	}
>> +
>> +	if (vga_default_device())
>> +		return;
>> +
>> +	vga_set_default_device(pdev);
>> +}
>> +DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_HUAWEI, 0x1711, hibmc_fixup_vgaarb);
>> -- 
>> 2.11.0
>>