From: Bjorn Helgaas <helgaas@kernel.org>
To: 隋景峰 <suijingfeng@loongson.cn>
Cc: Huacai Chen <chenhuacai@gmail.com>,
Huacai Chen <chenhuacai@loongson.cn>,
Bjorn Helgaas <bhelgaas@google.com>,
linux-pci <linux-pci@vger.kernel.org>,
Jiaxun Yang <jiaxun.yang@flygoat.com>
Subject: Re: Re: [PATCH 5/5] PCI: Support ASpeed VGA cards behind a misbehaving bridge
Date: Mon, 17 May 2021 22:09:17 -0500 [thread overview]
Message-ID: <20210518030917.GA72161@bjorn-Precision-5520> (raw)
In-Reply-To: <e82843c.42a4.1797d50c753.Coremail.suijingfeng@loongson.cn>
On Tue, May 18, 2021 at 10:31:56AM +0800, 隋景峰 wrote:
> > -----Original Messages-----
Wow, this is ugly (the ">" instead of ">"). Can you figure out
how to respond in the usual plain-text way?
> > From: "Bjorn Helgaas" <helgaas@kernel.org>
> > Sent Time: 2021-05-18 02:28:10 (Tuesday)
> > To: "Huacai Chen" <chenhuacai@gmail.com>
> > Cc: "Huacai Chen" <chenhuacai@loongson.cn>, "Bjorn Helgaas" <bhelgaas@google.com>, linux-pci <linux-pci@vger.kernel.org>, "Jiaxun Yang" <jiaxun.yang@flygoat.com>, "Jingfeng Sui" <suijingfeng@loongson.cn>
> > Subject: Re: [PATCH 5/5] PCI: Support ASpeed VGA cards behind a misbehaving bridge
> >
> > On Mon, May 17, 2021 at 08:53:43PM +0800, Huacai Chen wrote:
> > > On Sat, May 15, 2021 at 5:09 PM Huacai Chen <chenhuacai@gmail.com> wrote:
> > > > On Fri, May 14, 2021 at 11:10 PM Bjorn Helgaas <helgaas@kernel.org> wrote:
> > > > > On Fri, May 14, 2021 at 04:00:25PM +0800, Huacai Chen wrote:
> > > > > > According to PCI-to-PCI bridge spec, bit 3 of Bridge Control Register is
> > > > > > VGA Enable bit which modifies the response to VGA compatible addresses.
> > > > >
> > > > > The bridge spec is pretty old, and most of the content has been
> > > > > incorporated into the PCIe spec. I think you can cite "PCIe r5.0, sec
> > > > > 7.5.1.3.13" here instead.
> > > > >
> > > > > > If the VGA Enable bit is set, the bridge will decode and forward the
> > > > > > following accesses on the primary interface to the secondary interface.
> > > > >
> > > > > *Which* following accesses? The structure of English requires that if
> > > > > you say "the following accesses," you must continue by *listing* the
> > > > > accesses.
> > > > >
> > > > > > The ASpeed AST2500 hardward does not set the VGA Enable bit on its
> > > > > > bridge control register, which causes vgaarb subsystem don't think the
> > > > > > VGA card behind the bridge as a valid boot vga device.
> > > > >
> > > > > s/hardward/bridge/
> > > > > s/vga/VGA/ (also in code comments and dmesg strings below)
> > > > >
> > > > > From the code, it looks like AST2500 ([1a03:2000]) is a VGA device,
> > > > > since it apparently has a VGA class code. But here you say the
> > > > > AST2500 has a Bridge Control register, which suggests that it's a
> > > > > bridge. If AST2500 is some sort of combination that includes both a
> > > > > bridge and a VGA device, please outline that topology.
> > > > >
> > > > > But the hardware defect is that some bridges forward VGA accesses even
> > > > > though their VGA Enable bit is not set? The quirk should be attached
> > > > > to broken *bridges*, not to VGA devices.
> > > > >
> > > > > If a bridge forwards VGA accesses regardless of how its VGA Enable bit
> > > > > is set, that means VGA arbitration (in vgaarb.c) cannot work
> > > > > correctly, so merely setting the default VGA device once in a quirk is
> > > > > not sufficient. You would have to somehow disable any future attempts
> > > > > to use other VGA devices. Only the VGA device below this defective
> > > > > bridge is usable. Any other VGA devices in the system would be
> > > > > useless.
> > > > >
> > > > > > So we provide a quirk to fix Xorg auto-detection.
> > > > > >
> > > > > > See similar bug:
> > > > > >
> > > > > > https://patchwork.kernel.org/project/linux-pci/patch/20170619023528.11532-1-dja@axtens.net/
> > > > >
> > > > > This patch was never merged. If we merged a revised version, please
> > > > > cite the SHA1 instead.
> > > >
> > > > This patch has never merged, and I found that it is unnecessary after
> > > > commit a37c0f48950b56f6ef2ee637 ("vgaarb: Select a default VGA device
> > > > even if there's no legacy VGA"). Maybe this ASpeed patch is also
> > > > unnecessary. If it is still needed, I'll investigate the root cause.
> > >
> > > I found that vga_arb_device_init() and pcibios_init() are both wrapped
> > > by subsys_initcall(), which means their sequence is unpredictable. And
> > > unfortunately, in our platform vga_arb_device_init() is called before
> > > pcibios_init(), which makes vga_arb_device_init() fail to set a
> > > default vga device. This is the root cause why we thought that we
> > > still need a quirk for AST2500.
> >
> > Does this mean there is no hardware defect here? The VGA Enable bit
> > works correctly?
>
>
> The AST2500 BMC card we are using consist of a PCI-to-PCI Bridge (1a03:1150)
> and a PCI VGA device (1a03:2000). The value of the Bridge Control register
> in its PCI-to-PCI Bridge Configuration Registers is 0x0000. Thus, the VGA
> Enable bit in the Bridge Control register do not set, and the VGA
> Enable bit is not writable.
If VGA Enable is 0 and cannot be set to 1, the bridge should *never*
forward VGA accesses to its secondary bus. The generic VGA driver
that uses the legacy [mem 0xa0000-0xbffff] range should not work with
the VGA device at 05:00.0, and that device cannot participate in the
VGA arbitration scheme, which relies on the VGA Enable bit.
If you have a driver for 05:00.0 that doesn't need the legacy memory
range, it's possible that it may work. But VGA arbitration will be
broken, and if 05:00.0 needs to be initialized by an option ROM, that
probably won't work either.
If the 04:00.0 bridge *always* forwards VGA accesses, even though its
VGA Enable bit is always zero, then the bridge is broken. In that
case, the generic VGA driver should work with the 05:00.0 device, but
VGA arbitration will be limited. I'm not sure, but the arbiter
*might* be able to use the VGA Enable bit in the 00:0c.0 bridge to
control VGA access to 05:00.0. You wouldn't be able to have more than
one VGA device below 00:0c.0, and you may not be able have more than
one in the entire system.
> The topology of this BMC card is illustrated as following:
>
> /sys/devices/pci0000:00
> |-- 0000:00:0c.0
> | |-- class (0x060400)
> | |-- vendor (0x0014)
> | |-- device (0x7a09)
> | |-- ...
> | |-- 0000:04:00.0
> | | | -- class (0x060400)
> | | | -- device (0x1150)
> | | | -- vendor (0x1a03)
> | | | -- revision (0x04)
> | | | -- ...
> | | | -- 0000:05:00.0
> | | | | -- class (0x030000)
> | | | | -- device (0x2000)
> | | | | -- vendor (0x1a03)
> | | | | -- irq (51)
> | | | | -- i2c-6
> | | | | -- drm
> | | | | -- graphics
> | | | | -- ...
> | `-- uevent
> `-- ...
>
> The following information is getted from lspci -vvxx:
Generally it's better to use "sudo lspci -vvxx" so you decode all the
capabilities, too. But in this case, all we care about is the Bridge
Control register ("bridgectl"), which *is* included (and apparently is
set to 0, since it's decoded as "vga-").
> 04:00.0 PCI bridge: ASPEED Technology, Inc. AST1150 PCI-to-PCI Bridge (rev 04) (prog-if 00 [Normal decode])
> Control: I/O+ Mem+ BusMaster+ SpecCycle- MemWINV- VGASnoop- ParErr- Stepping- SERR- FastB2B- DisINTx-
> Status: Cap+ 66MHz- UDF- FastB2B- ParErr- DEVSEL=fast >TAbort- <tabort- <mabort-="">SERR- <perr- intx-="" latency:="" 0="" interrupt:="" pin="" a="" routed="" to="" irq="" 51="" numa="" node:="" bus:="" primary="04," secondary="05," subordinate="05," sec-latency="0" i="" o="" behind="" bridge:="" 00004000-00004fff="" memory="" 41000000-427fffff="" status:="" 66mhz+="" fastb2b-="" parerr-="" devsel="medium">TAbort- <tabort- <mabort-="" <serr-="" <perr-="" bridgectl:="" parity-="" serr-="" noisa-="" vga-="" mabort-="">Reset- FastB2B-
> PriDiscTmr- SecDiscTmr- DiscTmrStat- DiscTmrSERREn-
> Capabilities: <access denied="">
> 00: 03 1a 50 11 07 00 10 00 04 00 04 06 00 00 01 00
> 10: 00 00 00 00 00 00 00 00 04 05 05 00 41 41 20 02
> 20: 00 41 70 42 f1 ff 01 00 00 00 00 00 00 00 00 00
> 30: 00 00 00 00 50 00 00 00 00 00 00 00 63 01 00 00
>
> 05:00.0 VGA compatible controller: ASPEED Technology, Inc. ASPEED Graphics Family (rev 41) (prog-if 00 [VGA controller])
> Subsystem: ASPEED Technology, Inc. ASPEED Graphics Family
> Control: I/O+ Mem+ BusMaster+ SpecCycle- MemWINV- VGASnoop+ ParErr- Stepping- SERR- FastB2B- DisINTx-
> Status: Cap+ 66MHz- UDF- FastB2B- ParErr- DEVSEL=medium >TAbort- <tabort- <mabort-="">SERR- <perr- intx-="" latency:="" 0="" interrupt:="" pin="" a="" routed="" to="" irq="" 51="" numa="" node:="" region="" 0:="" memory="" at="" 41000000="" (32-bit,="" non-prefetchable)="" [size="16M]" 1:="" 42000000="" 2:="" i="" o="" ports="" 4000="" capabilities:="" <access="" denied="">
> Kernel driver in use: ast
> 00: 03 1a 00 20 27 00 10 02 41 00 00 03 00 00 00 00
> 10: 00 00 00 41 00 00 00 42 01 40 00 00 00 00 00 00
> 20: 00 00 00 00 00 00 00 00 00 00 00 00 03 1a 00 20
> 30: 00 00 00 00 40 00 00 00 00 00 00 00 63 01 00 00
next prev parent reply other threads:[~2021-05-18 3:09 UTC|newest]
Thread overview: 42+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-05-14 8:00 [PATCH 0/5] PCI: Loongson-related pci quirks Huacai Chen
2021-05-14 8:00 ` [PATCH 1/5] PCI/portdrv: Don't disable pci device during shutdown Huacai Chen
2021-05-14 14:20 ` Krzysztof Wilczyński
2021-05-14 16:09 ` Bjorn Helgaas
2021-05-15 3:38 ` Huacai Chen
2021-05-14 8:00 ` [PATCH 2/5] PCI: Move loongson pci quirks to quirks.c Huacai Chen
2021-05-14 8:00 ` [PATCH 3/5] PCI: Improve the mrrs quirk for LS7A Huacai Chen
2021-05-14 14:03 ` Krzysztof Wilczyński
2021-05-14 15:39 ` Bjorn Helgaas
2021-05-15 3:49 ` Huacai Chen
2021-05-15 6:22 ` Jiaxun Yang
2021-05-14 8:00 ` [PATCH 4/5] PCI: Add quirk for multifunction devices of LS7A Huacai Chen
2021-05-14 13:22 ` Krzysztof Wilczyński
2021-05-14 14:52 ` Jiaxun Yang
2021-05-15 3:52 ` Huacai Chen
2021-05-18 13:59 ` Bjorn Helgaas
2021-05-19 2:26 ` Huacai Chen
2021-05-19 3:05 ` Jiaxun Yang
2021-05-14 15:51 ` Bjorn Helgaas
2021-05-15 3:56 ` Huacai Chen
2021-05-14 8:00 ` [PATCH 5/5] PCI: Support ASpeed VGA cards behind a misbehaving bridge Huacai Chen
2021-05-14 13:56 ` Krzysztof Wilczyński
2021-05-14 15:10 ` Bjorn Helgaas
2021-05-15 9:09 ` Huacai Chen
2021-05-17 12:53 ` Huacai Chen
2021-05-17 18:28 ` Bjorn Helgaas
2021-05-18 2:31 ` 隋景峰
2021-05-18 3:09 ` Bjorn Helgaas [this message]
2021-05-18 9:30 ` suijingfeng
2021-05-18 19:31 ` Bjorn Helgaas
2021-05-18 7:13 ` Huacai Chen
2021-05-18 19:35 ` Bjorn Helgaas
2021-05-19 2:17 ` Huacai Chen
2021-05-19 19:33 ` Bjorn Helgaas
2021-05-25 11:03 ` Huacai Chen
2021-05-25 13:55 ` Bjorn Helgaas
2021-05-26 2:33 ` Huacai Chen
2021-05-26 2:33 ` Huacai Chen
2021-05-26 3:00 ` Dave Airlie
2021-05-26 3:00 ` Dave Airlie
2021-05-26 18:29 ` Bjorn Helgaas
2021-05-26 18:29 ` Bjorn Helgaas
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20210518030917.GA72161@bjorn-Precision-5520 \
--to=helgaas@kernel.org \
--cc=bhelgaas@google.com \
--cc=chenhuacai@gmail.com \
--cc=chenhuacai@loongson.cn \
--cc=jiaxun.yang@flygoat.com \
--cc=linux-pci@vger.kernel.org \
--cc=suijingfeng@loongson.cn \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.