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: 39+ 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 3:00 ` Dave Airlie
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 a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).