Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1480276 > unrolled thread
| Started by | Tomasz Nowicki <tn@semihalf.com> |
|---|---|
| First post | 2016-09-09 21:30 +0200 |
| Last post | 2016-09-23 01:10 +0200 |
| Articles | 20 on this page of 41 — 9 participants |
Back to article view | Back to linux.kernel
[PATCH V6 0/5] ECAM quirks handling for ARM64 platforms Tomasz Nowicki <tn@semihalf.com> - 2016-09-09 21:30 +0200
[PATCH V6 4/5] PCI: thunder: Enable ACPI PCI controller for ThunderX pass2.x silicon version Tomasz Nowicki <tn@semihalf.com> - 2016-09-09 21:30 +0200
Re: [PATCH V6 4/5] PCI: thunder: Enable ACPI PCI controller for ThunderX pass2.x silicon version Bjorn Helgaas <helgaas@kernel.org> - 2016-09-19 17:50 +0200
Re: [PATCH V6 4/5] PCI: thunder: Enable ACPI PCI controller for ThunderX pass2.x silicon version Tomasz Nowicki <tn@semihalf.com> - 2016-09-20 09:10 +0200
Re: [PATCH V6 4/5] PCI: thunder: Enable ACPI PCI controller for ThunderX pass2.x silicon version Bjorn Helgaas <helgaas@kernel.org> - 2016-09-20 15:20 +0200
Re: [PATCH V6 4/5] PCI: thunder: Enable ACPI PCI controller for ThunderX pass2.x silicon version Tomasz Nowicki <tn@semihalf.com> - 2016-09-21 10:10 +0200
[PATCH V6 3/5] PCI: thunder-pem: Allow to probe PEM-specific register range for ACPI case Tomasz Nowicki <tn@semihalf.com> - 2016-09-09 21:30 +0200
Re: [PATCH V6 3/5] PCI: thunder-pem: Allow to probe PEM-specific register range for ACPI case Bjorn Helgaas <helgaas@kernel.org> - 2016-09-19 20:10 +0200
Re: [PATCH V6 3/5] PCI: thunder-pem: Allow to probe PEM-specific register range for ACPI case Tomasz Nowicki <tn@semihalf.com> - 2016-09-20 09:30 +0200
Re: [PATCH V6 3/5] PCI: thunder-pem: Allow to probe PEM-specific register range for ACPI case Bjorn Helgaas <helgaas@kernel.org> - 2016-09-20 15:40 +0200
Re: [PATCH V6 3/5] PCI: thunder-pem: Allow to probe PEM-specific register range for ACPI case Ard Biesheuvel <ard.biesheuvel@linaro.org> - 2016-09-20 15:50 +0200
Re: [PATCH V6 3/5] PCI: thunder-pem: Allow to probe PEM-specific register range for ACPI case Bjorn Helgaas <helgaas@kernel.org> - 2016-09-20 16:10 +0200
Re: [PATCH V6 3/5] PCI: thunder-pem: Allow to probe PEM-specific register range for ACPI case Ard Biesheuvel <ard.biesheuvel@linaro.org> - 2016-09-20 17:10 +0200
Re: [PATCH V6 3/5] PCI: thunder-pem: Allow to probe PEM-specific register range for ACPI case Bjorn Helgaas <helgaas@kernel.org> - 2016-09-20 21:20 +0200
Re: [PATCH V6 3/5] PCI: thunder-pem: Allow to probe PEM-specific register range for ACPI case Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> - 2016-09-21 16:10 +0200
Re: [PATCH V6 3/5] PCI: thunder-pem: Allow to probe PEM-specific register range for ACPI case Bjorn Helgaas <helgaas@kernel.org> - 2016-09-21 20:10 +0200
Re: [PATCH V6 3/5] PCI: thunder-pem: Allow to probe PEM-specific register range for ACPI case Duc Dang <dhdang@apm.com> - 2016-09-21 21:10 +0200
Re: [PATCH V6 3/5] PCI: thunder-pem: Allow to probe PEM-specific register range for ACPI case Bjorn Helgaas <helgaas@kernel.org> - 2016-09-21 21:20 +0200
Re: [PATCH V6 3/5] PCI: thunder-pem: Allow to probe PEM-specific register range for ACPI case Tomasz Nowicki <tn@semihalf.com> - 2016-09-23 13:00 +0200
Re: [PATCH V6 3/5] PCI: thunder-pem: Allow to probe PEM-specific register range for ACPI case Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> - 2016-09-22 11:50 +0200
RE: [PATCH V6 3/5] PCI: thunder-pem: Allow to probe PEM-specific register range for ACPI case Gabriele Paoloni <gabriele.paoloni@huawei.com> - 2016-09-22 13:20 +0200
Re: [PATCH V6 3/5] PCI: thunder-pem: Allow to probe PEM-specific register range for ACPI case Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> - 2016-09-22 14:50 +0200
Re: [PATCH V6 3/5] PCI: thunder-pem: Allow to probe PEM-specific register range for ACPI case Bjorn Helgaas <helgaas@kernel.org> - 2016-09-22 20:40 +0200
Re: [PATCH V6 3/5] PCI: thunder-pem: Allow to probe PEM-specific register range for ACPI case Bjorn Helgaas <helgaas@kernel.org> - 2016-09-23 00:20 +0200
Re: [PATCH V6 3/5] PCI: thunder-pem: Allow to probe PEM-specific register range for ACPI case Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> - 2016-09-23 12:20 +0200
RE: [PATCH V6 3/5] PCI: thunder-pem: Allow to probe PEM-specific register range for ACPI case Gabriele Paoloni <gabriele.paoloni@huawei.com> - 2016-09-23 13:00 +0200
Re: [PATCH V6 3/5] PCI: thunder-pem: Allow to probe PEM-specific register range for ACPI case Christopher Covington <cov@codeaurora.org> - 2016-09-22 16:30 +0200
RE: [PATCH V6 3/5] PCI: thunder-pem: Allow to probe PEM-specific register range for ACPI case Gabriele Paoloni <gabriele.paoloni@huawei.com> - 2016-09-21 16:20 +0200
Re: [PATCH V6 3/5] PCI: thunder-pem: Allow to probe PEM-specific register range for ACPI case Bjorn Helgaas <helgaas@kernel.org> - 2016-09-21 21:00 +0200
RE: [PATCH V6 3/5] PCI: thunder-pem: Allow to probe PEM-specific register range for ACPI case Gabriele Paoloni <gabriele.paoloni@huawei.com> - 2016-09-22 13:20 +0200
[PATCH V6 1/5] PCI/ACPI: Extend pci_mcfg_lookup() responsibilities Tomasz Nowicki <tn@semihalf.com> - 2016-09-09 21:30 +0200
Re: [PATCH V6 0/5] ECAM quirks handling for ARM64 platforms Tomasz Nowicki <tn@semihalf.com> - 2016-09-09 21:40 +0200
Re: [PATCH V6 0/5] ECAM quirks handling for ARM64 platforms Bjorn Helgaas <helgaas@kernel.org> - 2016-09-20 21:30 +0200
Re: [PATCH V6 0/5] ECAM quirks handling for ARM64 platforms cov@codeaurora.org - 2016-09-21 03:20 +0200
Re: [PATCH V6 0/5] ECAM quirks handling for ARM64 platforms Bjorn Helgaas <helgaas@kernel.org> - 2016-09-21 15:20 +0200
Re: [PATCH V6 0/5] ECAM quirks handling for ARM64 platforms Sinan Kaya <okaya@codeaurora.org> - 2016-09-21 16:10 +0200
Re: [PATCH V6 0/5] ECAM quirks handling for ARM64 platforms Sinan Kaya <okaya@codeaurora.org> - 2016-09-21 19:40 +0200
Re: [PATCH V6 0/5] ECAM quirks handling for ARM64 platforms Bjorn Helgaas <helgaas@kernel.org> - 2016-09-21 19:40 +0200
[PATCHv2] PCI: QDF2432 32 bit config space accessors Christopher Covington <cov@codeaurora.org> - 2016-09-22 00:40 +0200
Re: [PATCH V6 0/5] ECAM quirks handling for ARM64 platforms Christopher Covington <cov@codeaurora.org> - 2016-09-22 00:50 +0200
Re: [PATCH V6 0/5] ECAM quirks handling for ARM64 platforms Bjorn Helgaas <helgaas@kernel.org> - 2016-09-23 01:10 +0200
Page 2 of 3 — ← Prev page 1 [2] 3 Next page →
| From | Gabriele Paoloni <gabriele.paoloni@huawei.com> |
|---|---|
| Date | 2016-09-22 13:20 +0200 |
| Subject | RE: [PATCH V6 3/5] PCI: thunder-pem: Allow to probe PEM-specific register range for ACPI case |
| Message-ID | <skacO-5BK-21@gated-at.bofh.it> |
| In reply to | #1488699 |
Hi Lorenzo, Bjorn
> -----Original Message-----
> From: Lorenzo Pieralisi [mailto:lorenzo.pieralisi@arm.com]
> Sent: 22 September 2016 10:50
> To: Bjorn Helgaas
> Cc: Ard Biesheuvel; Tomasz Nowicki; David Daney; Will Deacon; Catalin
> Marinas; Rafael Wysocki; Arnd Bergmann; Hanjun Guo; Sinan Kaya;
> Jayachandran C; Christopher Covington; Duc Dang; Robert Richter; Marcin
> Wojtas; Liviu Dudau; Wangyijing; Mark Salter; linux-
> pci@vger.kernel.org; linux-arm-kernel@lists.infradead.org; Linaro ACPI
> Mailman List; Jon Masters; Andrea Gallo; Jeremy Linton; liudongdong
> (C); Gabriele Paoloni; Jeff Hugo; linux-acpi@vger.kernel.org; linux-
> kernel@vger.kernel.org; Rafael J. Wysocki
> Subject: Re: [PATCH V6 3/5] PCI: thunder-pem: Allow to probe PEM-
> specific register range for ACPI case
>
> On Wed, Sep 21, 2016 at 01:04:57PM -0500, Bjorn Helgaas wrote:
> > On Wed, Sep 21, 2016 at 03:05:49PM +0100, Lorenzo Pieralisi wrote:
> > > On Tue, Sep 20, 2016 at 02:17:44PM -0500, Bjorn Helgaas wrote:
> > > > On Tue, Sep 20, 2016 at 04:09:25PM +0100, Ard Biesheuvel wrote:
> > >
> > > [...]
> > >
> > > > > None of these platforms can be fixed entirely in software, and
> given
> > > > > that we will not be adding quirks for new broken hardware, we
> should
> > > > > ask ourselves whether having two versions of a quirk, i.e., one
> for
> > > > > broken hardware + currently shipping firmware, and one for the
> same
> > > > > broken hardware with fixed firmware is really an improvement
> over what
> > > > > has been proposed here.
> > > >
> > > > We're talking about two completely different types of quirks:
> > > >
> > > > 1) MCFG quirks to use memory-mapped config space that doesn't
> quite
> > > > conform to the ECAM model in the PCIe spec, and
> > > >
> > > > 2) Some yet-to-be-determined method to describe address space
> > > > consumed by a bridge.
> > > >
> > > > The first two patches of this series are a nice implementation
> for 1).
> > > > The third patch (ThunderX-specific) is one possibility for 2),
> but I
> > > > don't like it because there's no way for generic software like
> the
> > > > ACPI core to discover these resources.
> > >
> > > Ok, so basically this means that to implement (2) we need to assign
> > > some sort of _HID to these quirky PCI bridges (so that we know what
> > > device they represent and we can retrieve their _CRS). I take from
> > > this discussion that the goal is to make sure that all non-config
> > > resources have to be declared through _CRS device objects, which is
> > > fine but that requires a FW update (unless we can fabricate ACPI
> > > devices and corresponding _CRS in the kernel whenever we match a
> > > given MCFG table signature).
> >
> > All resources consumed by ACPI devices should be declared through
> > _CRS. If you want to fabricate ACPI devices or _CRS via kernel
> > quirks, that's fine with me. This could be triggered via MCFG
> > signature, DMI info, host bridge _HID, etc.
>
> I think the PNP quirk approach + PNP0c02 resource put forward by Gab
> is enough.
Great thanks as we take a final decision I will ask Dogndgong to submit
another RFC based on this approach
>
> > > We discussed this already and I think we should make a decision:
> > >
> > > http://lists.infradead.org/pipermail/linux-arm-kernel/2016-
> March/414722.html
> > >
> > > > > > I'd like to step back and come up with some understanding of
> how
> > > > > > non-broken firmware *should* deal with this issue. Then, if
> we *do*
> > > > > > work around this particular broken firmware in the kernel, it
> would be
> > > > > > nice to do it in a way that fits in with that understanding.
> > > > > >
> > > > > > For example, if a companion ACPI device is the preferred
> solution, an
> > > > > > ACPI quirk could fabricate a device with the required
> resources. That
> > > > > > would address the problem closer to the source and make it
> more likely
> > > > > > that the rest of the system will work correctly: /proc/iomem
> could
> > > > > > make sense, things that look at _CRS generically would work
> (e.g,
> > > > > > /sys/, an admittedly hypothetical "lsacpi", etc.)
> > > > > >
> > > > > > Hard-coding stuff in drivers is a point solution that doesn't
> provide
> > > > > > any guidance for future platforms and makes it likely that
> the hack
> > > > > > will get copied into even more drivers.
> > > > > >
> > > > >
> > > > > OK, I see. But the guidance for future platforms should be 'do
> not
> > > > > rely on quirks', and what I am arguing here is that the more we
> polish
> > > > > up this code and make it clean and reusable, the more likely it
> is
> > > > > that will end up getting abused by new broken hardware that we
> set out
> > > > > to reject entirely in the first place.
> > > > >
> > > > > So of course, if the quirk involves claiming resources, let's
> make
> > > > > sure that this occurs in the cleanest and most compliant way
> possible.
> > > > > But any factoring/reuse concerns other than for the current
> crop of
> > > > > broken hardware should be avoided imo.
> > > >
> > > > If future hardware is completely ECAM-compliant and we don't need
> any
> > > > more MCFG quirks, that would be great.
> > >
> > > Yes.
> > >
> > > > But we'll still need to describe that memory-mapped config space
> > > > somewhere. If that's done with PNP0C02 or similar devices (as is
> done
> > > > on my x86 laptop), we'd be all set.
> > >
> > > I am not sure I understand what you mean here. Are you referring
> > > to MCFG regions reported as PNP0c02 resources through its _CRS ?
> >
> > Yes. PCI Firmware Spec r3.0, Table 4-2, note 2 says address ranges
> > reported via MCFG or _CBA should be reserved by _CRS of a PNP0C02
> > device.
>
> Ok, that's agreed. It goes without saying that since you are quoting
> the PCI spec, if FW fails to report MCFG regions in a PNP0c02 device
> _CRS I will consider that a FW bug.
>
> > > IIUC PNP0C02 is a reservation mechanism, but it does not help us
> > > associate its _CRS to a specific PCI host bridge instance, right ?
> >
> > Gab proposed a hierarchy that *would* associate a PNP0C02 device with
> > a PCI bridge:
> >
> > Device (PCI1) {
> > Name (_HID, "HISI0080") // PCI Express Root Bridge
> > Name (_CID, "PNP0A03") // Compatible PCI Root Bridge
> > Method (_CRS, 0, Serialized) { // Root complex resources
> (windows) }
> > Device (RES0) {
> > Name (_HID, "HISI0081") // HiSi PCIe RC config base address
> > Name (_CID, "PNP0C02") // Motherboard reserved resource
> > Name (_CRS, ResourceTemplate () { ... }
> > }
> > }
> >
> > That's a possibility. The PCI Firmware Spec suggests putting RES0 at
> > the root (under \_SB), but I don't know why.
> >
> > Putting it at the root means we couldn't generically associate it
> with
> > a bridge, although I could imagine something like this:
> >
> > Device (RES1) {
> > Name (_HID, "HISI0081") // HiSi PCIe RC config base address
> > Name (_CID, "PNP0C02") // Motherboard reserved resource
> > Name (_CRS, ResourceTemplate () { ... }
> > Method (BRDG) { "PCI1" } // hand-wavy ASL
> > }
> > Device (PCI1) {
> > Name (_HID, "HISI0080") // PCI Express Root Bridge
> > Name (_CID, "PNP0A03") // Compatible PCI Root Bridge
> > Method (_CRS, 0, Serialized) { // Root complex resources
> (windows) }
> > }
> >
> > Where you could search PNP0C02 devices for a cookie that matched the
> > host bridge.o
>
> Ok, I am fine with both and I think we are converging, but the way
> to solve this problem has to be uniform for all ARM partners (and
> not only ARM). Two points here:
>
> 1) Adding a device/subdevice allows people to add a _CRS reporting the
> non-window bridge resources. Fine. It also allows people to chuck in
> there all sorts of _DSD properties to describe their PCI host bridge
> as it is done with DT properties (those _DSD can contain eg clocks
> etc.), this may be tempting (so that they can reuse the same DT
> driver and do not have to update their firmware) but I want to be
> clear here: that must not happen. So, a subdevice with a _CRS to
> report resources, yes, but it will stop there.
> 2) It is unclear to me how to formalize the above. People should not
> write FW by reading the PCI mailing list, so these guidelines have
> to
> be written, somehow. I do not want to standardize quirks, I want
> to prevent random ACPI table content, which is different.
> Should I report this to the ACPI spec working group ? If we do
> not do that everyone will go solve this problem as they deem fit.
>
Do we really need to formalize this?
As we discussed in the Linaro call at the moment we have few vendors
that need quirks and we want to avoid promoting/accepting quirks for
the future.
At the time of the call I think we decided to informally accept a set
of quirks for the current platforms and reject any other quirk coming
after a certain date/kernel version (this to be decided).
I am not sure if there is a way to document/formalize a temporary
exception from the rule...
Thanks
Gab
> [...]
>
> > > For FW that is immutable I really do not see what we can do apart
> > > from hardcoding the non-config resources (consumed by a bridge),
> > > somehow.
> >
> > Right. Well, I assume you mean we should hard-code "non-window
> > resources consumed directly by a bridge". If firmware in the field
> is
> > broken, we should work around it, and that may mean hard-coding some
> > resources.
> >
> > My point is that the hard-coding should not be buried in a driver
> > where it's invisible to the rest of the kernel. If we hard-code it
> in
> > a quirk that adds _CRS entries, then the kernel will work just like
> it
> > would if the firmware had been correct in the first place. The
> > resource will appear in /sys/devices/pnp*/*/resources and
> /proc/iomem,
> > and if we ever used _SRS to assign or move ACPI devices, we would
> know
> > to avoid the bridge resource.
>
> We are in complete agreement here.
>
> Thanks,
> Lorenzo
[toc] | [prev] | [next] | [standalone]
| From | Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> |
|---|---|
| Date | 2016-09-22 14:50 +0200 |
| Subject | Re: [PATCH V6 3/5] PCI: thunder-pem: Allow to probe PEM-specific register range for ACPI case |
| Message-ID | <skbBT-6t2-1@gated-at.bofh.it> |
| In reply to | #1488785 |
On Thu, Sep 22, 2016 at 11:10:13AM +0000, Gabriele Paoloni wrote:
> Hi Lorenzo, Bjorn
>
> > -----Original Message-----
> > From: Lorenzo Pieralisi [mailto:lorenzo.pieralisi@arm.com]
> > Sent: 22 September 2016 10:50
> > To: Bjorn Helgaas
> > Cc: Ard Biesheuvel; Tomasz Nowicki; David Daney; Will Deacon; Catalin
> > Marinas; Rafael Wysocki; Arnd Bergmann; Hanjun Guo; Sinan Kaya;
> > Jayachandran C; Christopher Covington; Duc Dang; Robert Richter; Marcin
> > Wojtas; Liviu Dudau; Wangyijing; Mark Salter; linux-
> > pci@vger.kernel.org; linux-arm-kernel@lists.infradead.org; Linaro ACPI
> > Mailman List; Jon Masters; Andrea Gallo; Jeremy Linton; liudongdong
> > (C); Gabriele Paoloni; Jeff Hugo; linux-acpi@vger.kernel.org; linux-
> > kernel@vger.kernel.org; Rafael J. Wysocki
> > Subject: Re: [PATCH V6 3/5] PCI: thunder-pem: Allow to probe PEM-
> > specific register range for ACPI case
> >
> > On Wed, Sep 21, 2016 at 01:04:57PM -0500, Bjorn Helgaas wrote:
> > > On Wed, Sep 21, 2016 at 03:05:49PM +0100, Lorenzo Pieralisi wrote:
> > > > On Tue, Sep 20, 2016 at 02:17:44PM -0500, Bjorn Helgaas wrote:
> > > > > On Tue, Sep 20, 2016 at 04:09:25PM +0100, Ard Biesheuvel wrote:
> > > >
> > > > [...]
> > > >
> > > > > > None of these platforms can be fixed entirely in software, and
> > given
> > > > > > that we will not be adding quirks for new broken hardware, we
> > should
> > > > > > ask ourselves whether having two versions of a quirk, i.e., one
> > for
> > > > > > broken hardware + currently shipping firmware, and one for the
> > same
> > > > > > broken hardware with fixed firmware is really an improvement
> > over what
> > > > > > has been proposed here.
> > > > >
> > > > > We're talking about two completely different types of quirks:
> > > > >
> > > > > 1) MCFG quirks to use memory-mapped config space that doesn't
> > quite
> > > > > conform to the ECAM model in the PCIe spec, and
> > > > >
> > > > > 2) Some yet-to-be-determined method to describe address space
> > > > > consumed by a bridge.
> > > > >
> > > > > The first two patches of this series are a nice implementation
> > for 1).
> > > > > The third patch (ThunderX-specific) is one possibility for 2),
> > but I
> > > > > don't like it because there's no way for generic software like
> > the
> > > > > ACPI core to discover these resources.
> > > >
> > > > Ok, so basically this means that to implement (2) we need to assign
> > > > some sort of _HID to these quirky PCI bridges (so that we know what
> > > > device they represent and we can retrieve their _CRS). I take from
> > > > this discussion that the goal is to make sure that all non-config
> > > > resources have to be declared through _CRS device objects, which is
> > > > fine but that requires a FW update (unless we can fabricate ACPI
> > > > devices and corresponding _CRS in the kernel whenever we match a
> > > > given MCFG table signature).
> > >
> > > All resources consumed by ACPI devices should be declared through
> > > _CRS. If you want to fabricate ACPI devices or _CRS via kernel
> > > quirks, that's fine with me. This could be triggered via MCFG
> > > signature, DMI info, host bridge _HID, etc.
> >
> > I think the PNP quirk approach + PNP0c02 resource put forward by Gab
> > is enough.
>
> Great thanks as we take a final decision I will ask Dogndgong to submit
> another RFC based on this approach
>
> >
> > > > We discussed this already and I think we should make a decision:
> > > >
> > > > http://lists.infradead.org/pipermail/linux-arm-kernel/2016-
> > March/414722.html
> > > >
> > > > > > > I'd like to step back and come up with some understanding of
> > how
> > > > > > > non-broken firmware *should* deal with this issue. Then, if
> > we *do*
> > > > > > > work around this particular broken firmware in the kernel, it
> > would be
> > > > > > > nice to do it in a way that fits in with that understanding.
> > > > > > >
> > > > > > > For example, if a companion ACPI device is the preferred
> > solution, an
> > > > > > > ACPI quirk could fabricate a device with the required
> > resources. That
> > > > > > > would address the problem closer to the source and make it
> > more likely
> > > > > > > that the rest of the system will work correctly: /proc/iomem
> > could
> > > > > > > make sense, things that look at _CRS generically would work
> > (e.g,
> > > > > > > /sys/, an admittedly hypothetical "lsacpi", etc.)
> > > > > > >
> > > > > > > Hard-coding stuff in drivers is a point solution that doesn't
> > provide
> > > > > > > any guidance for future platforms and makes it likely that
> > the hack
> > > > > > > will get copied into even more drivers.
> > > > > > >
> > > > > >
> > > > > > OK, I see. But the guidance for future platforms should be 'do
> > not
> > > > > > rely on quirks', and what I am arguing here is that the more we
> > polish
> > > > > > up this code and make it clean and reusable, the more likely it
> > is
> > > > > > that will end up getting abused by new broken hardware that we
> > set out
> > > > > > to reject entirely in the first place.
> > > > > >
> > > > > > So of course, if the quirk involves claiming resources, let's
> > make
> > > > > > sure that this occurs in the cleanest and most compliant way
> > possible.
> > > > > > But any factoring/reuse concerns other than for the current
> > crop of
> > > > > > broken hardware should be avoided imo.
> > > > >
> > > > > If future hardware is completely ECAM-compliant and we don't need
> > any
> > > > > more MCFG quirks, that would be great.
> > > >
> > > > Yes.
> > > >
> > > > > But we'll still need to describe that memory-mapped config space
> > > > > somewhere. If that's done with PNP0C02 or similar devices (as is
> > done
> > > > > on my x86 laptop), we'd be all set.
> > > >
> > > > I am not sure I understand what you mean here. Are you referring
> > > > to MCFG regions reported as PNP0c02 resources through its _CRS ?
> > >
> > > Yes. PCI Firmware Spec r3.0, Table 4-2, note 2 says address ranges
> > > reported via MCFG or _CBA should be reserved by _CRS of a PNP0C02
> > > device.
> >
> > Ok, that's agreed. It goes without saying that since you are quoting
> > the PCI spec, if FW fails to report MCFG regions in a PNP0c02 device
> > _CRS I will consider that a FW bug.
> >
> > > > IIUC PNP0C02 is a reservation mechanism, but it does not help us
> > > > associate its _CRS to a specific PCI host bridge instance, right ?
> > >
> > > Gab proposed a hierarchy that *would* associate a PNP0C02 device with
> > > a PCI bridge:
> > >
> > > Device (PCI1) {
> > > Name (_HID, "HISI0080") // PCI Express Root Bridge
> > > Name (_CID, "PNP0A03") // Compatible PCI Root Bridge
> > > Method (_CRS, 0, Serialized) { // Root complex resources
> > (windows) }
> > > Device (RES0) {
> > > Name (_HID, "HISI0081") // HiSi PCIe RC config base address
> > > Name (_CID, "PNP0C02") // Motherboard reserved resource
> > > Name (_CRS, ResourceTemplate () { ... }
> > > }
> > > }
> > >
> > > That's a possibility. The PCI Firmware Spec suggests putting RES0 at
> > > the root (under \_SB), but I don't know why.
> > >
> > > Putting it at the root means we couldn't generically associate it
> > with
> > > a bridge, although I could imagine something like this:
> > >
> > > Device (RES1) {
> > > Name (_HID, "HISI0081") // HiSi PCIe RC config base address
> > > Name (_CID, "PNP0C02") // Motherboard reserved resource
> > > Name (_CRS, ResourceTemplate () { ... }
> > > Method (BRDG) { "PCI1" } // hand-wavy ASL
> > > }
> > > Device (PCI1) {
> > > Name (_HID, "HISI0080") // PCI Express Root Bridge
> > > Name (_CID, "PNP0A03") // Compatible PCI Root Bridge
> > > Method (_CRS, 0, Serialized) { // Root complex resources
> > (windows) }
> > > }
> > >
> > > Where you could search PNP0C02 devices for a cookie that matched the
> > > host bridge.o
> >
> > Ok, I am fine with both and I think we are converging, but the way
> > to solve this problem has to be uniform for all ARM partners (and
> > not only ARM). Two points here:
> >
> > 1) Adding a device/subdevice allows people to add a _CRS reporting the
> > non-window bridge resources. Fine. It also allows people to chuck in
> > there all sorts of _DSD properties to describe their PCI host bridge
> > as it is done with DT properties (those _DSD can contain eg clocks
> > etc.), this may be tempting (so that they can reuse the same DT
> > driver and do not have to update their firmware) but I want to be
> > clear here: that must not happen. So, a subdevice with a _CRS to
> > report resources, yes, but it will stop there.
> > 2) It is unclear to me how to formalize the above. People should not
> > write FW by reading the PCI mailing list, so these guidelines have
> > to
> > be written, somehow. I do not want to standardize quirks, I want
> > to prevent random ACPI table content, which is different.
> > Should I report this to the ACPI spec working group ? If we do
> > not do that everyone will go solve this problem as they deem fit.
> >
>
> Do we really need to formalize this?
>
> As we discussed in the Linaro call at the moment we have few vendors
> that need quirks and we want to avoid promoting/accepting quirks for
> the future.
>
> At the time of the call I think we decided to informally accept a set
> of quirks for the current platforms and reject any other quirk coming
> after a certain date/kernel version (this to be decided).
>
> I am not sure if there is a way to document/formalize a temporary
> exception from the rule...
- (1) will be enforced.
- We do not know whether PNP0c02 can be used in non-root devices _CRS
- Are we sure (given that we are implementing this to make sure we are
able to validate resources) that it is valid to have a subdevice with
a _CRS whose resources are not contained in its parent _CRS address
space (because that's exactly the case for these quirks) ?
That's what I mean by formalizing, I want to know how PNP0c02 should
be used. We all want platforms with quirks to be enabled asap but only
if we stick to the ACPI specifications. On top of that, with the
bindings above, the kernel would end up creating a platform device for
the "fake" device with a _CRS approach, which is questionable.
Lorenzo
>
> Thanks
>
> Gab
>
>
> > [...]
> >
> > > > For FW that is immutable I really do not see what we can do apart
> > > > from hardcoding the non-config resources (consumed by a bridge),
> > > > somehow.
> > >
> > > Right. Well, I assume you mean we should hard-code "non-window
> > > resources consumed directly by a bridge". If firmware in the field
> > is
> > > broken, we should work around it, and that may mean hard-coding some
> > > resources.
> > >
> > > My point is that the hard-coding should not be buried in a driver
> > > where it's invisible to the rest of the kernel. If we hard-code it
> > in
> > > a quirk that adds _CRS entries, then the kernel will work just like
> > it
> > > would if the firmware had been correct in the first place. The
> > > resource will appear in /sys/devices/pnp*/*/resources and
> > /proc/iomem,
> > > and if we ever used _SRS to assign or move ACPI devices, we would
> > know
> > > to avoid the bridge resource.
> >
> > We are in complete agreement here.
> >
> > Thanks,
> > Lorenzo
>
[toc] | [prev] | [next] | [standalone]
| From | Bjorn Helgaas <helgaas@kernel.org> |
|---|---|
| Date | 2016-09-22 20:40 +0200 |
| Subject | Re: [PATCH V6 3/5] PCI: thunder-pem: Allow to probe PEM-specific register range for ACPI case |
| Message-ID | <skh4B-1rX-11@gated-at.bofh.it> |
| In reply to | #1488854 |
On Thu, Sep 22, 2016 at 01:44:46PM +0100, Lorenzo Pieralisi wrote:
> On Thu, Sep 22, 2016 at 11:10:13AM +0000, Gabriele Paoloni wrote:
> > Hi Lorenzo, Bjorn
> >
> > > -----Original Message-----
> > > From: Lorenzo Pieralisi [mailto:lorenzo.pieralisi@arm.com]
> > > Sent: 22 September 2016 10:50
> > > To: Bjorn Helgaas
> > > Cc: Ard Biesheuvel; Tomasz Nowicki; David Daney; Will Deacon; Catalin
> > > Marinas; Rafael Wysocki; Arnd Bergmann; Hanjun Guo; Sinan Kaya;
> > > Jayachandran C; Christopher Covington; Duc Dang; Robert Richter; Marcin
> > > Wojtas; Liviu Dudau; Wangyijing; Mark Salter; linux-
> > > pci@vger.kernel.org; linux-arm-kernel@lists.infradead.org; Linaro ACPI
> > > Mailman List; Jon Masters; Andrea Gallo; Jeremy Linton; liudongdong
> > > (C); Gabriele Paoloni; Jeff Hugo; linux-acpi@vger.kernel.org; linux-
> > > kernel@vger.kernel.org; Rafael J. Wysocki
> > > Subject: Re: [PATCH V6 3/5] PCI: thunder-pem: Allow to probe PEM-
> > > specific register range for ACPI case
> > >
> > > On Wed, Sep 21, 2016 at 01:04:57PM -0500, Bjorn Helgaas wrote:
> > > > On Wed, Sep 21, 2016 at 03:05:49PM +0100, Lorenzo Pieralisi wrote:
> > > > > On Tue, Sep 20, 2016 at 02:17:44PM -0500, Bjorn Helgaas wrote:
> > > > > > On Tue, Sep 20, 2016 at 04:09:25PM +0100, Ard Biesheuvel wrote:
> > > > >
> > > > > [...]
> > > > >
> > > > > > > None of these platforms can be fixed entirely in software, and
> > > given
> > > > > > > that we will not be adding quirks for new broken hardware, we
> > > should
> > > > > > > ask ourselves whether having two versions of a quirk, i.e., one
> > > for
> > > > > > > broken hardware + currently shipping firmware, and one for the
> > > same
> > > > > > > broken hardware with fixed firmware is really an improvement
> > > over what
> > > > > > > has been proposed here.
> > > > > >
> > > > > > We're talking about two completely different types of quirks:
> > > > > >
> > > > > > 1) MCFG quirks to use memory-mapped config space that doesn't
> > > quite
> > > > > > conform to the ECAM model in the PCIe spec, and
> > > > > >
> > > > > > 2) Some yet-to-be-determined method to describe address space
> > > > > > consumed by a bridge.
> > > > > >
> > > > > > The first two patches of this series are a nice implementation
> > > for 1).
> > > > > > The third patch (ThunderX-specific) is one possibility for 2),
> > > but I
> > > > > > don't like it because there's no way for generic software like
> > > the
> > > > > > ACPI core to discover these resources.
> > > > >
> > > > > Ok, so basically this means that to implement (2) we need to assign
> > > > > some sort of _HID to these quirky PCI bridges (so that we know what
> > > > > device they represent and we can retrieve their _CRS). I take from
> > > > > this discussion that the goal is to make sure that all non-config
> > > > > resources have to be declared through _CRS device objects, which is
> > > > > fine but that requires a FW update (unless we can fabricate ACPI
> > > > > devices and corresponding _CRS in the kernel whenever we match a
> > > > > given MCFG table signature).
> > > >
> > > > All resources consumed by ACPI devices should be declared through
> > > > _CRS. If you want to fabricate ACPI devices or _CRS via kernel
> > > > quirks, that's fine with me. This could be triggered via MCFG
> > > > signature, DMI info, host bridge _HID, etc.
> > >
> > > I think the PNP quirk approach + PNP0c02 resource put forward by Gab
> > > is enough.
> >
> > Great thanks as we take a final decision I will ask Dogndgong to submit
> > another RFC based on this approach
> >
> > >
> > > > > We discussed this already and I think we should make a decision:
> > > > >
> > > > > http://lists.infradead.org/pipermail/linux-arm-kernel/2016-
> > > March/414722.html
> > > > >
> > > > > > > > I'd like to step back and come up with some understanding of
> > > how
> > > > > > > > non-broken firmware *should* deal with this issue. Then, if
> > > we *do*
> > > > > > > > work around this particular broken firmware in the kernel, it
> > > would be
> > > > > > > > nice to do it in a way that fits in with that understanding.
> > > > > > > >
> > > > > > > > For example, if a companion ACPI device is the preferred
> > > solution, an
> > > > > > > > ACPI quirk could fabricate a device with the required
> > > resources. That
> > > > > > > > would address the problem closer to the source and make it
> > > more likely
> > > > > > > > that the rest of the system will work correctly: /proc/iomem
> > > could
> > > > > > > > make sense, things that look at _CRS generically would work
> > > (e.g,
> > > > > > > > /sys/, an admittedly hypothetical "lsacpi", etc.)
> > > > > > > >
> > > > > > > > Hard-coding stuff in drivers is a point solution that doesn't
> > > provide
> > > > > > > > any guidance for future platforms and makes it likely that
> > > the hack
> > > > > > > > will get copied into even more drivers.
> > > > > > > >
> > > > > > >
> > > > > > > OK, I see. But the guidance for future platforms should be 'do
> > > not
> > > > > > > rely on quirks', and what I am arguing here is that the more we
> > > polish
> > > > > > > up this code and make it clean and reusable, the more likely it
> > > is
> > > > > > > that will end up getting abused by new broken hardware that we
> > > set out
> > > > > > > to reject entirely in the first place.
> > > > > > >
> > > > > > > So of course, if the quirk involves claiming resources, let's
> > > make
> > > > > > > sure that this occurs in the cleanest and most compliant way
> > > possible.
> > > > > > > But any factoring/reuse concerns other than for the current
> > > crop of
> > > > > > > broken hardware should be avoided imo.
> > > > > >
> > > > > > If future hardware is completely ECAM-compliant and we don't need
> > > any
> > > > > > more MCFG quirks, that would be great.
> > > > >
> > > > > Yes.
> > > > >
> > > > > > But we'll still need to describe that memory-mapped config space
> > > > > > somewhere. If that's done with PNP0C02 or similar devices (as is
> > > done
> > > > > > on my x86 laptop), we'd be all set.
> > > > >
> > > > > I am not sure I understand what you mean here. Are you referring
> > > > > to MCFG regions reported as PNP0c02 resources through its _CRS ?
> > > >
> > > > Yes. PCI Firmware Spec r3.0, Table 4-2, note 2 says address ranges
> > > > reported via MCFG or _CBA should be reserved by _CRS of a PNP0C02
> > > > device.
> > >
> > > Ok, that's agreed. It goes without saying that since you are quoting
> > > the PCI spec, if FW fails to report MCFG regions in a PNP0c02 device
> > > _CRS I will consider that a FW bug.
> > >
> > > > > IIUC PNP0C02 is a reservation mechanism, but it does not help us
> > > > > associate its _CRS to a specific PCI host bridge instance, right ?
> > > >
> > > > Gab proposed a hierarchy that *would* associate a PNP0C02 device with
> > > > a PCI bridge:
> > > >
> > > > Device (PCI1) {
> > > > Name (_HID, "HISI0080") // PCI Express Root Bridge
> > > > Name (_CID, "PNP0A03") // Compatible PCI Root Bridge
> > > > Method (_CRS, 0, Serialized) { // Root complex resources
> > > (windows) }
> > > > Device (RES0) {
> > > > Name (_HID, "HISI0081") // HiSi PCIe RC config base address
> > > > Name (_CID, "PNP0C02") // Motherboard reserved resource
> > > > Name (_CRS, ResourceTemplate () { ... }
> > > > }
> > > > }
> > > >
> > > > That's a possibility. The PCI Firmware Spec suggests putting RES0 at
> > > > the root (under \_SB), but I don't know why.
> > > >
> > > > Putting it at the root means we couldn't generically associate it
> > > with
> > > > a bridge, although I could imagine something like this:
> > > >
> > > > Device (RES1) {
> > > > Name (_HID, "HISI0081") // HiSi PCIe RC config base address
> > > > Name (_CID, "PNP0C02") // Motherboard reserved resource
> > > > Name (_CRS, ResourceTemplate () { ... }
> > > > Method (BRDG) { "PCI1" } // hand-wavy ASL
> > > > }
> > > > Device (PCI1) {
> > > > Name (_HID, "HISI0080") // PCI Express Root Bridge
> > > > Name (_CID, "PNP0A03") // Compatible PCI Root Bridge
> > > > Method (_CRS, 0, Serialized) { // Root complex resources
> > > (windows) }
> > > > }
> > > >
> > > > Where you could search PNP0C02 devices for a cookie that matched the
> > > > host bridge.o
> > >
> > > Ok, I am fine with both and I think we are converging, but the way
> > > to solve this problem has to be uniform for all ARM partners (and
> > > not only ARM). Two points here:
> > >
> > > 1) Adding a device/subdevice allows people to add a _CRS reporting the
> > > non-window bridge resources. Fine. It also allows people to chuck in
> > > there all sorts of _DSD properties to describe their PCI host bridge
> > > as it is done with DT properties (those _DSD can contain eg clocks
> > > etc.), this may be tempting (so that they can reuse the same DT
> > > driver and do not have to update their firmware) but I want to be
> > > clear here: that must not happen. So, a subdevice with a _CRS to
> > > report resources, yes, but it will stop there.
> > > 2) It is unclear to me how to formalize the above. People should not
> > > write FW by reading the PCI mailing list, so these guidelines have
> > > to
> > > be written, somehow. I do not want to standardize quirks, I want
> > > to prevent random ACPI table content, which is different.
> > > Should I report this to the ACPI spec working group ? If we do
> > > not do that everyone will go solve this problem as they deem fit.
> > >
> >
> > Do we really need to formalize this?
> >
> > As we discussed in the Linaro call at the moment we have few vendors
> > that need quirks and we want to avoid promoting/accepting quirks for
> > the future.
> >
> > At the time of the call I think we decided to informally accept a set
> > of quirks for the current platforms and reject any other quirk coming
> > after a certain date/kernel version (this to be decided).
> >
> > I am not sure if there is a way to document/formalize a temporary
> > exception from the rule...
>
> - (1) will be enforced.
I'm not sure it's necessary or possible to enforce a "no future
quirks" rule. For one thing, there's already a pretty strong
incentive to avoid quirks: if your hardware doesn't require quirks,
it works with OSes already in the field.
MCFG quirks allow us to use the generic ACPI pci_root.c driver even if
the hardware doesn't support ECAM quite according to the spec.
PNP0C02 usage is a workaround for the failure of the Consumer/Producer
bit. PNP0C02 quirks compensate for firmware that doesn't describe
resource usage accurately. It's possible the ACPI spec folks could
come up with a better Consumer/Producer workaround, if that's needed.
Apparently x86 hasn't needed it yet.
If people add _DSD methods for clocks or whatnot, the hardware won't
work with the generic pci_root.c driver, so there's already an
incentive for avoiding them. x86 has managed without such methods;
arm64 should be able to do the same.
> - We do not know whether PNP0c02 can be used in non-root devices _CRS
> - Are we sure (given that we are implementing this to make sure we are
> able to validate resources) that it is valid to have a subdevice with
> a _CRS whose resources are not contained in its parent _CRS address
> space (because that's exactly the case for these quirks) ?
>
> That's what I mean by formalizing, I want to know how PNP0c02 should
> be used. We all want platforms with quirks to be enabled asap but only
> if we stick to the ACPI specifications. On top of that, with the
> bindings above, the kernel would end up creating a platform device for
> the "fake" device with a _CRS approach, which is questionable.
> > > [...]
> > >
> > > > > For FW that is immutable I really do not see what we can do apart
> > > > > from hardcoding the non-config resources (consumed by a bridge),
> > > > > somehow.
> > > >
> > > > Right. Well, I assume you mean we should hard-code "non-window
> > > > resources consumed directly by a bridge". If firmware in the field
> > > is
> > > > broken, we should work around it, and that may mean hard-coding some
> > > > resources.
> > > >
> > > > My point is that the hard-coding should not be buried in a driver
> > > > where it's invisible to the rest of the kernel. If we hard-code it
> > > in
> > > > a quirk that adds _CRS entries, then the kernel will work just like
> > > it
> > > > would if the firmware had been correct in the first place. The
> > > > resource will appear in /sys/devices/pnp*/*/resources and
> > > /proc/iomem,
> > > > and if we ever used _SRS to assign or move ACPI devices, we would
> > > know
> > > > to avoid the bridge resource.
> > >
> > > We are in complete agreement here.
> > >
> > > Thanks,
> > > Lorenzo
> >
[toc] | [prev] | [next] | [standalone]
| From | Bjorn Helgaas <helgaas@kernel.org> |
|---|---|
| Date | 2016-09-23 00:20 +0200 |
| Subject | Re: [PATCH V6 3/5] PCI: thunder-pem: Allow to probe PEM-specific register range for ACPI case |
| Message-ID | <skkvv-3IV-13@gated-at.bofh.it> |
| In reply to | #1489326 |
On Thu, Sep 22, 2016 at 01:31:01PM -0500, Bjorn Helgaas wrote:
> On Thu, Sep 22, 2016 at 01:44:46PM +0100, Lorenzo Pieralisi wrote:
> > On Thu, Sep 22, 2016 at 11:10:13AM +0000, Gabriele Paoloni wrote:
> > > Hi Lorenzo, Bjorn
> > >
> > > > -----Original Message-----
> > > > From: Lorenzo Pieralisi [mailto:lorenzo.pieralisi@arm.com]
> > > > Sent: 22 September 2016 10:50
> > > > To: Bjorn Helgaas
> > > > Cc: Ard Biesheuvel; Tomasz Nowicki; David Daney; Will Deacon; Catalin
> > > > Marinas; Rafael Wysocki; Arnd Bergmann; Hanjun Guo; Sinan Kaya;
> > > > Jayachandran C; Christopher Covington; Duc Dang; Robert Richter; Marcin
> > > > Wojtas; Liviu Dudau; Wangyijing; Mark Salter; linux-
> > > > pci@vger.kernel.org; linux-arm-kernel@lists.infradead.org; Linaro ACPI
> > > > Mailman List; Jon Masters; Andrea Gallo; Jeremy Linton; liudongdong
> > > > (C); Gabriele Paoloni; Jeff Hugo; linux-acpi@vger.kernel.org; linux-
> > > > kernel@vger.kernel.org; Rafael J. Wysocki
> > > > Subject: Re: [PATCH V6 3/5] PCI: thunder-pem: Allow to probe PEM-
> > > > specific register range for ACPI case
> > > >
> > > > On Wed, Sep 21, 2016 at 01:04:57PM -0500, Bjorn Helgaas wrote:
> > > > > On Wed, Sep 21, 2016 at 03:05:49PM +0100, Lorenzo Pieralisi wrote:
> > > > > > On Tue, Sep 20, 2016 at 02:17:44PM -0500, Bjorn Helgaas wrote:
> > > > > > > On Tue, Sep 20, 2016 at 04:09:25PM +0100, Ard Biesheuvel wrote:
> > > > > >
> > > > > > [...]
> > > > > >
> > > > > > > > None of these platforms can be fixed entirely in software, and
> > > > given
> > > > > > > > that we will not be adding quirks for new broken hardware, we
> > > > should
> > > > > > > > ask ourselves whether having two versions of a quirk, i.e., one
> > > > for
> > > > > > > > broken hardware + currently shipping firmware, and one for the
> > > > same
> > > > > > > > broken hardware with fixed firmware is really an improvement
> > > > over what
> > > > > > > > has been proposed here.
> > > > > > >
> > > > > > > We're talking about two completely different types of quirks:
> > > > > > >
> > > > > > > 1) MCFG quirks to use memory-mapped config space that doesn't
> > > > quite
> > > > > > > conform to the ECAM model in the PCIe spec, and
> > > > > > >
> > > > > > > 2) Some yet-to-be-determined method to describe address space
> > > > > > > consumed by a bridge.
> > > > > > >
> > > > > > > The first two patches of this series are a nice implementation
> > > > for 1).
> > > > > > > The third patch (ThunderX-specific) is one possibility for 2),
> > > > but I
> > > > > > > don't like it because there's no way for generic software like
> > > > the
> > > > > > > ACPI core to discover these resources.
> > > > > >
> > > > > > Ok, so basically this means that to implement (2) we need to assign
> > > > > > some sort of _HID to these quirky PCI bridges (so that we know what
> > > > > > device they represent and we can retrieve their _CRS). I take from
> > > > > > this discussion that the goal is to make sure that all non-config
> > > > > > resources have to be declared through _CRS device objects, which is
> > > > > > fine but that requires a FW update (unless we can fabricate ACPI
> > > > > > devices and corresponding _CRS in the kernel whenever we match a
> > > > > > given MCFG table signature).
> > > > >
> > > > > All resources consumed by ACPI devices should be declared through
> > > > > _CRS. If you want to fabricate ACPI devices or _CRS via kernel
> > > > > quirks, that's fine with me. This could be triggered via MCFG
> > > > > signature, DMI info, host bridge _HID, etc.
> > > >
> > > > I think the PNP quirk approach + PNP0c02 resource put forward by Gab
> > > > is enough.
> > >
> > > Great thanks as we take a final decision I will ask Dogndgong to submit
> > > another RFC based on this approach
> > >
> > > >
> > > > > > We discussed this already and I think we should make a decision:
> > > > > >
> > > > > > http://lists.infradead.org/pipermail/linux-arm-kernel/2016-
> > > > March/414722.html
> > > > > >
> > > > > > > > > I'd like to step back and come up with some understanding of
> > > > how
> > > > > > > > > non-broken firmware *should* deal with this issue. Then, if
> > > > we *do*
> > > > > > > > > work around this particular broken firmware in the kernel, it
> > > > would be
> > > > > > > > > nice to do it in a way that fits in with that understanding.
> > > > > > > > >
> > > > > > > > > For example, if a companion ACPI device is the preferred
> > > > solution, an
> > > > > > > > > ACPI quirk could fabricate a device with the required
> > > > resources. That
> > > > > > > > > would address the problem closer to the source and make it
> > > > more likely
> > > > > > > > > that the rest of the system will work correctly: /proc/iomem
> > > > could
> > > > > > > > > make sense, things that look at _CRS generically would work
> > > > (e.g,
> > > > > > > > > /sys/, an admittedly hypothetical "lsacpi", etc.)
> > > > > > > > >
> > > > > > > > > Hard-coding stuff in drivers is a point solution that doesn't
> > > > provide
> > > > > > > > > any guidance for future platforms and makes it likely that
> > > > the hack
> > > > > > > > > will get copied into even more drivers.
> > > > > > > > >
> > > > > > > >
> > > > > > > > OK, I see. But the guidance for future platforms should be 'do
> > > > not
> > > > > > > > rely on quirks', and what I am arguing here is that the more we
> > > > polish
> > > > > > > > up this code and make it clean and reusable, the more likely it
> > > > is
> > > > > > > > that will end up getting abused by new broken hardware that we
> > > > set out
> > > > > > > > to reject entirely in the first place.
> > > > > > > >
> > > > > > > > So of course, if the quirk involves claiming resources, let's
> > > > make
> > > > > > > > sure that this occurs in the cleanest and most compliant way
> > > > possible.
> > > > > > > > But any factoring/reuse concerns other than for the current
> > > > crop of
> > > > > > > > broken hardware should be avoided imo.
> > > > > > >
> > > > > > > If future hardware is completely ECAM-compliant and we don't need
> > > > any
> > > > > > > more MCFG quirks, that would be great.
> > > > > >
> > > > > > Yes.
> > > > > >
> > > > > > > But we'll still need to describe that memory-mapped config space
> > > > > > > somewhere. If that's done with PNP0C02 or similar devices (as is
> > > > done
> > > > > > > on my x86 laptop), we'd be all set.
> > > > > >
> > > > > > I am not sure I understand what you mean here. Are you referring
> > > > > > to MCFG regions reported as PNP0c02 resources through its _CRS ?
> > > > >
> > > > > Yes. PCI Firmware Spec r3.0, Table 4-2, note 2 says address ranges
> > > > > reported via MCFG or _CBA should be reserved by _CRS of a PNP0C02
> > > > > device.
> > > >
> > > > Ok, that's agreed. It goes without saying that since you are quoting
> > > > the PCI spec, if FW fails to report MCFG regions in a PNP0c02 device
> > > > _CRS I will consider that a FW bug.
> > > >
> > > > > > IIUC PNP0C02 is a reservation mechanism, but it does not help us
> > > > > > associate its _CRS to a specific PCI host bridge instance, right ?
> > > > >
> > > > > Gab proposed a hierarchy that *would* associate a PNP0C02 device with
> > > > > a PCI bridge:
> > > > >
> > > > > Device (PCI1) {
> > > > > Name (_HID, "HISI0080") // PCI Express Root Bridge
> > > > > Name (_CID, "PNP0A03") // Compatible PCI Root Bridge
> > > > > Method (_CRS, 0, Serialized) { // Root complex resources
> > > > (windows) }
> > > > > Device (RES0) {
> > > > > Name (_HID, "HISI0081") // HiSi PCIe RC config base address
> > > > > Name (_CID, "PNP0C02") // Motherboard reserved resource
> > > > > Name (_CRS, ResourceTemplate () { ... }
> > > > > }
> > > > > }
> > > > >
> > > > > That's a possibility. The PCI Firmware Spec suggests putting RES0 at
> > > > > the root (under \_SB), but I don't know why.
> > > > >
> > > > > Putting it at the root means we couldn't generically associate it
> > > > with
> > > > > a bridge, although I could imagine something like this:
> > > > >
> > > > > Device (RES1) {
> > > > > Name (_HID, "HISI0081") // HiSi PCIe RC config base address
> > > > > Name (_CID, "PNP0C02") // Motherboard reserved resource
> > > > > Name (_CRS, ResourceTemplate () { ... }
> > > > > Method (BRDG) { "PCI1" } // hand-wavy ASL
> > > > > }
> > > > > Device (PCI1) {
> > > > > Name (_HID, "HISI0080") // PCI Express Root Bridge
> > > > > Name (_CID, "PNP0A03") // Compatible PCI Root Bridge
> > > > > Method (_CRS, 0, Serialized) { // Root complex resources
> > > > (windows) }
> > > > > }
> > > > >
> > > > > Where you could search PNP0C02 devices for a cookie that matched the
> > > > > host bridge.o
> > > >
> > > > Ok, I am fine with both and I think we are converging, but the way
> > > > to solve this problem has to be uniform for all ARM partners (and
> > > > not only ARM). Two points here:
> > > >
> > > > 1) Adding a device/subdevice allows people to add a _CRS reporting the
> > > > non-window bridge resources. Fine. It also allows people to chuck in
> > > > there all sorts of _DSD properties to describe their PCI host bridge
> > > > as it is done with DT properties (those _DSD can contain eg clocks
> > > > etc.), this may be tempting (so that they can reuse the same DT
> > > > driver and do not have to update their firmware) but I want to be
> > > > clear here: that must not happen. So, a subdevice with a _CRS to
> > > > report resources, yes, but it will stop there.
> > > > 2) It is unclear to me how to formalize the above. People should not
> > > > write FW by reading the PCI mailing list, so these guidelines have
> > > > to
> > > > be written, somehow. I do not want to standardize quirks, I want
> > > > to prevent random ACPI table content, which is different.
> > > > Should I report this to the ACPI spec working group ? If we do
> > > > not do that everyone will go solve this problem as they deem fit.
> > > >
> > >
> > > Do we really need to formalize this?
> > >
> > > As we discussed in the Linaro call at the moment we have few vendors
> > > that need quirks and we want to avoid promoting/accepting quirks for
> > > the future.
> > >
> > > At the time of the call I think we decided to informally accept a set
> > > of quirks for the current platforms and reject any other quirk coming
> > > after a certain date/kernel version (this to be decided).
> > >
> > > I am not sure if there is a way to document/formalize a temporary
> > > exception from the rule...
> >
> > - (1) will be enforced.
>
> I'm not sure it's necessary or possible to enforce a "no future
> quirks" rule. For one thing, there's already a pretty strong
> incentive to avoid quirks: if your hardware doesn't require quirks,
> it works with OSes already in the field.
>
> MCFG quirks allow us to use the generic ACPI pci_root.c driver even if
> the hardware doesn't support ECAM quite according to the spec.
>
> PNP0C02 usage is a workaround for the failure of the Consumer/Producer
> bit. PNP0C02 quirks compensate for firmware that doesn't describe
> resource usage accurately. It's possible the ACPI spec folks could
> come up with a better Consumer/Producer workaround, if that's needed.
> Apparently x86 hasn't needed it yet.
>
> If people add _DSD methods for clocks or whatnot, the hardware won't
> work with the generic pci_root.c driver, so there's already an
> incentive for avoiding them. x86 has managed without such methods;
> arm64 should be able to do the same.
Re-reading this, I'm afraid my response sounds a little dismissive,
and I feel like I'm missing some important information. So I
apologize if I missed your whole point, Lorenzo.
Bjorn
[toc] | [prev] | [next] | [standalone]
| From | Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> |
|---|---|
| Date | 2016-09-23 12:20 +0200 |
| Subject | Re: [PATCH V6 3/5] PCI: thunder-pem: Allow to probe PEM-specific register range for ACPI case |
| Message-ID | <skvKh-2mq-7@gated-at.bofh.it> |
| In reply to | #1489614 |
[+ Zhang Rui]
On Thu, Sep 22, 2016 at 05:10:42PM -0500, Bjorn Helgaas wrote:
> On Thu, Sep 22, 2016 at 01:31:01PM -0500, Bjorn Helgaas wrote:
> > On Thu, Sep 22, 2016 at 01:44:46PM +0100, Lorenzo Pieralisi wrote:
> > > On Thu, Sep 22, 2016 at 11:10:13AM +0000, Gabriele Paoloni wrote:
> > > > Hi Lorenzo, Bjorn
> > > >
> > > > > -----Original Message-----
> > > > > From: Lorenzo Pieralisi [mailto:lorenzo.pieralisi@arm.com]
> > > > > Sent: 22 September 2016 10:50
> > > > > To: Bjorn Helgaas
> > > > > Cc: Ard Biesheuvel; Tomasz Nowicki; David Daney; Will Deacon; Catalin
> > > > > Marinas; Rafael Wysocki; Arnd Bergmann; Hanjun Guo; Sinan Kaya;
> > > > > Jayachandran C; Christopher Covington; Duc Dang; Robert Richter; Marcin
> > > > > Wojtas; Liviu Dudau; Wangyijing; Mark Salter; linux-
> > > > > pci@vger.kernel.org; linux-arm-kernel@lists.infradead.org; Linaro ACPI
> > > > > Mailman List; Jon Masters; Andrea Gallo; Jeremy Linton; liudongdong
> > > > > (C); Gabriele Paoloni; Jeff Hugo; linux-acpi@vger.kernel.org; linux-
> > > > > kernel@vger.kernel.org; Rafael J. Wysocki
> > > > > Subject: Re: [PATCH V6 3/5] PCI: thunder-pem: Allow to probe PEM-
> > > > > specific register range for ACPI case
> > > > >
> > > > > On Wed, Sep 21, 2016 at 01:04:57PM -0500, Bjorn Helgaas wrote:
> > > > > > On Wed, Sep 21, 2016 at 03:05:49PM +0100, Lorenzo Pieralisi wrote:
> > > > > > > On Tue, Sep 20, 2016 at 02:17:44PM -0500, Bjorn Helgaas wrote:
> > > > > > > > On Tue, Sep 20, 2016 at 04:09:25PM +0100, Ard Biesheuvel wrote:
> > > > > > >
> > > > > > > [...]
> > > > > > >
> > > > > > > > > None of these platforms can be fixed entirely in software, and
> > > > > given
> > > > > > > > > that we will not be adding quirks for new broken hardware, we
> > > > > should
> > > > > > > > > ask ourselves whether having two versions of a quirk, i.e., one
> > > > > for
> > > > > > > > > broken hardware + currently shipping firmware, and one for the
> > > > > same
> > > > > > > > > broken hardware with fixed firmware is really an improvement
> > > > > over what
> > > > > > > > > has been proposed here.
> > > > > > > >
> > > > > > > > We're talking about two completely different types of quirks:
> > > > > > > >
> > > > > > > > 1) MCFG quirks to use memory-mapped config space that doesn't
> > > > > quite
> > > > > > > > conform to the ECAM model in the PCIe spec, and
> > > > > > > >
> > > > > > > > 2) Some yet-to-be-determined method to describe address space
> > > > > > > > consumed by a bridge.
> > > > > > > >
> > > > > > > > The first two patches of this series are a nice implementation
> > > > > for 1).
> > > > > > > > The third patch (ThunderX-specific) is one possibility for 2),
> > > > > but I
> > > > > > > > don't like it because there's no way for generic software like
> > > > > the
> > > > > > > > ACPI core to discover these resources.
> > > > > > >
> > > > > > > Ok, so basically this means that to implement (2) we need to assign
> > > > > > > some sort of _HID to these quirky PCI bridges (so that we know what
> > > > > > > device they represent and we can retrieve their _CRS). I take from
> > > > > > > this discussion that the goal is to make sure that all non-config
> > > > > > > resources have to be declared through _CRS device objects, which is
> > > > > > > fine but that requires a FW update (unless we can fabricate ACPI
> > > > > > > devices and corresponding _CRS in the kernel whenever we match a
> > > > > > > given MCFG table signature).
> > > > > >
> > > > > > All resources consumed by ACPI devices should be declared through
> > > > > > _CRS. If you want to fabricate ACPI devices or _CRS via kernel
> > > > > > quirks, that's fine with me. This could be triggered via MCFG
> > > > > > signature, DMI info, host bridge _HID, etc.
> > > > >
> > > > > I think the PNP quirk approach + PNP0c02 resource put forward by Gab
> > > > > is enough.
> > > >
> > > > Great thanks as we take a final decision I will ask Dogndgong to submit
> > > > another RFC based on this approach
> > > >
> > > > >
> > > > > > > We discussed this already and I think we should make a decision:
> > > > > > >
> > > > > > > http://lists.infradead.org/pipermail/linux-arm-kernel/2016-
> > > > > March/414722.html
> > > > > > >
> > > > > > > > > > I'd like to step back and come up with some understanding of
> > > > > how
> > > > > > > > > > non-broken firmware *should* deal with this issue. Then, if
> > > > > we *do*
> > > > > > > > > > work around this particular broken firmware in the kernel, it
> > > > > would be
> > > > > > > > > > nice to do it in a way that fits in with that understanding.
> > > > > > > > > >
> > > > > > > > > > For example, if a companion ACPI device is the preferred
> > > > > solution, an
> > > > > > > > > > ACPI quirk could fabricate a device with the required
> > > > > resources. That
> > > > > > > > > > would address the problem closer to the source and make it
> > > > > more likely
> > > > > > > > > > that the rest of the system will work correctly: /proc/iomem
> > > > > could
> > > > > > > > > > make sense, things that look at _CRS generically would work
> > > > > (e.g,
> > > > > > > > > > /sys/, an admittedly hypothetical "lsacpi", etc.)
> > > > > > > > > >
> > > > > > > > > > Hard-coding stuff in drivers is a point solution that doesn't
> > > > > provide
> > > > > > > > > > any guidance for future platforms and makes it likely that
> > > > > the hack
> > > > > > > > > > will get copied into even more drivers.
> > > > > > > > > >
> > > > > > > > >
> > > > > > > > > OK, I see. But the guidance for future platforms should be 'do
> > > > > not
> > > > > > > > > rely on quirks', and what I am arguing here is that the more we
> > > > > polish
> > > > > > > > > up this code and make it clean and reusable, the more likely it
> > > > > is
> > > > > > > > > that will end up getting abused by new broken hardware that we
> > > > > set out
> > > > > > > > > to reject entirely in the first place.
> > > > > > > > >
> > > > > > > > > So of course, if the quirk involves claiming resources, let's
> > > > > make
> > > > > > > > > sure that this occurs in the cleanest and most compliant way
> > > > > possible.
> > > > > > > > > But any factoring/reuse concerns other than for the current
> > > > > crop of
> > > > > > > > > broken hardware should be avoided imo.
> > > > > > > >
> > > > > > > > If future hardware is completely ECAM-compliant and we don't need
> > > > > any
> > > > > > > > more MCFG quirks, that would be great.
> > > > > > >
> > > > > > > Yes.
> > > > > > >
> > > > > > > > But we'll still need to describe that memory-mapped config space
> > > > > > > > somewhere. If that's done with PNP0C02 or similar devices (as is
> > > > > done
> > > > > > > > on my x86 laptop), we'd be all set.
> > > > > > >
> > > > > > > I am not sure I understand what you mean here. Are you referring
> > > > > > > to MCFG regions reported as PNP0c02 resources through its _CRS ?
> > > > > >
> > > > > > Yes. PCI Firmware Spec r3.0, Table 4-2, note 2 says address ranges
> > > > > > reported via MCFG or _CBA should be reserved by _CRS of a PNP0C02
> > > > > > device.
> > > > >
> > > > > Ok, that's agreed. It goes without saying that since you are quoting
> > > > > the PCI spec, if FW fails to report MCFG regions in a PNP0c02 device
> > > > > _CRS I will consider that a FW bug.
> > > > >
> > > > > > > IIUC PNP0C02 is a reservation mechanism, but it does not help us
> > > > > > > associate its _CRS to a specific PCI host bridge instance, right ?
> > > > > >
> > > > > > Gab proposed a hierarchy that *would* associate a PNP0C02 device with
> > > > > > a PCI bridge:
> > > > > >
> > > > > > Device (PCI1) {
> > > > > > Name (_HID, "HISI0080") // PCI Express Root Bridge
> > > > > > Name (_CID, "PNP0A03") // Compatible PCI Root Bridge
> > > > > > Method (_CRS, 0, Serialized) { // Root complex resources
> > > > > (windows) }
> > > > > > Device (RES0) {
> > > > > > Name (_HID, "HISI0081") // HiSi PCIe RC config base address
> > > > > > Name (_CID, "PNP0C02") // Motherboard reserved resource
> > > > > > Name (_CRS, ResourceTemplate () { ... }
> > > > > > }
> > > > > > }
> > > > > >
> > > > > > That's a possibility. The PCI Firmware Spec suggests putting RES0 at
> > > > > > the root (under \_SB), but I don't know why.
> > > > > >
> > > > > > Putting it at the root means we couldn't generically associate it
> > > > > with
> > > > > > a bridge, although I could imagine something like this:
> > > > > >
> > > > > > Device (RES1) {
> > > > > > Name (_HID, "HISI0081") // HiSi PCIe RC config base address
> > > > > > Name (_CID, "PNP0C02") // Motherboard reserved resource
> > > > > > Name (_CRS, ResourceTemplate () { ... }
> > > > > > Method (BRDG) { "PCI1" } // hand-wavy ASL
> > > > > > }
> > > > > > Device (PCI1) {
> > > > > > Name (_HID, "HISI0080") // PCI Express Root Bridge
> > > > > > Name (_CID, "PNP0A03") // Compatible PCI Root Bridge
> > > > > > Method (_CRS, 0, Serialized) { // Root complex resources
> > > > > (windows) }
> > > > > > }
> > > > > >
> > > > > > Where you could search PNP0C02 devices for a cookie that matched the
> > > > > > host bridge.o
> > > > >
> > > > > Ok, I am fine with both and I think we are converging, but the way
> > > > > to solve this problem has to be uniform for all ARM partners (and
> > > > > not only ARM). Two points here:
> > > > >
> > > > > 1) Adding a device/subdevice allows people to add a _CRS reporting the
> > > > > non-window bridge resources. Fine. It also allows people to chuck in
> > > > > there all sorts of _DSD properties to describe their PCI host bridge
> > > > > as it is done with DT properties (those _DSD can contain eg clocks
> > > > > etc.), this may be tempting (so that they can reuse the same DT
> > > > > driver and do not have to update their firmware) but I want to be
> > > > > clear here: that must not happen. So, a subdevice with a _CRS to
> > > > > report resources, yes, but it will stop there.
> > > > > 2) It is unclear to me how to formalize the above. People should not
> > > > > write FW by reading the PCI mailing list, so these guidelines have
> > > > > to
> > > > > be written, somehow. I do not want to standardize quirks, I want
> > > > > to prevent random ACPI table content, which is different.
> > > > > Should I report this to the ACPI spec working group ? If we do
> > > > > not do that everyone will go solve this problem as they deem fit.
> > > > >
> > > >
> > > > Do we really need to formalize this?
> > > >
> > > > As we discussed in the Linaro call at the moment we have few vendors
> > > > that need quirks and we want to avoid promoting/accepting quirks for
> > > > the future.
> > > >
> > > > At the time of the call I think we decided to informally accept a set
> > > > of quirks for the current platforms and reject any other quirk coming
> > > > after a certain date/kernel version (this to be decided).
> > > >
> > > > I am not sure if there is a way to document/formalize a temporary
> > > > exception from the rule...
> > >
> > > - (1) will be enforced.
> >
> > I'm not sure it's necessary or possible to enforce a "no future
> > quirks" rule. For one thing, there's already a pretty strong
> > incentive to avoid quirks: if your hardware doesn't require quirks,
> > it works with OSes already in the field.
> >
> > MCFG quirks allow us to use the generic ACPI pci_root.c driver even if
> > the hardware doesn't support ECAM quite according to the spec.
> >
> > PNP0C02 usage is a workaround for the failure of the Consumer/Producer
> > bit. PNP0C02 quirks compensate for firmware that doesn't describe
> > resource usage accurately. It's possible the ACPI spec folks could
> > come up with a better Consumer/Producer workaround, if that's needed.
> > Apparently x86 hasn't needed it yet.
> >
> > If people add _DSD methods for clocks or whatnot, the hardware won't
> > work with the generic pci_root.c driver, so there's already an
> > incentive for avoiding them. x86 has managed without such methods;
> > arm64 should be able to do the same.
>
> Re-reading this, I'm afraid my response sounds a little dismissive,
> and I feel like I'm missing some important information. So I
> apologize if I missed your whole point, Lorenzo.
No you are spot on, I just wanted to emphasize, given that we are
adding an _HID and a subdevice, that developer should not be tempted
to use it to match against a PCI host driver to reuse the DT code,
we should not use the quirk mechanism as a backdoor to re-using DT
drivers in ACPI context.
Anyway, there is a review process to spot these possible misuses,
mine was just a heads-up, quirks will happen, I just do not want
to wreak the standard ACPI PCI firmware model to support them.
Given that there are already PNP0c02 bindings out there where the
PNP0c02 is used as in Gab's example:
https://patchwork.kernel.org/patch/4757111/
I think the only pending question I have is whether we are allowed
to define a PNP0A03 subdevice with a _CRS resource space that is
not contained in its parent _CRS, if we answer this question I
think we are done.
I will raise the PNP0c02 usage issue with the ASWG anyway.
Thanks !
Lorenzo
[toc] | [prev] | [next] | [standalone]
| From | Gabriele Paoloni <gabriele.paoloni@huawei.com> |
|---|---|
| Date | 2016-09-23 13:00 +0200 |
| Subject | RE: [PATCH V6 3/5] PCI: thunder-pem: Allow to probe PEM-specific register range for ACPI case |
| Message-ID | <skwmZ-2zw-1@gated-at.bofh.it> |
| In reply to | #1489921 |
Hi Lorenzo
> -----Original Message-----
> From: linux-kernel-owner@vger.kernel.org [mailto:linux-kernel-
> owner@vger.kernel.org] On Behalf Of Lorenzo Pieralisi
> Sent: 23 September 2016 11:12
> To: Bjorn Helgaas
> Cc: Gabriele Paoloni; Ard Biesheuvel; Tomasz Nowicki; David Daney; Will
> Deacon; Catalin Marinas; Rafael Wysocki; Arnd Bergmann; Hanjun Guo;
> Sinan Kaya; Jayachandran C; Christopher Covington; Duc Dang; Robert
> Richter; Marcin Wojtas; Liviu Dudau; Wangyijing; Mark Salter; linux-
> pci@vger.kernel.org; linux-arm-kernel@lists.infradead.org; Linaro ACPI
> Mailman List; Jon Masters; Andrea Gallo; Jeremy Linton; liudongdong
> (C); Jeff Hugo; linux-acpi@vger.kernel.org; linux-
> kernel@vger.kernel.org; Rafael J. Wysocki; rui.zhang@intel.com
> Subject: Re: [PATCH V6 3/5] PCI: thunder-pem: Allow to probe PEM-
> specific register range for ACPI case
>
> [+ Zhang Rui]
>
> On Thu, Sep 22, 2016 at 05:10:42PM -0500, Bjorn Helgaas wrote:
> > On Thu, Sep 22, 2016 at 01:31:01PM -0500, Bjorn Helgaas wrote:
> > > On Thu, Sep 22, 2016 at 01:44:46PM +0100, Lorenzo Pieralisi wrote:
> > > > On Thu, Sep 22, 2016 at 11:10:13AM +0000, Gabriele Paoloni wrote:
> > > > > Hi Lorenzo, Bjorn
> > > > >
> > > > > > -----Original Message-----
> > > > > > From: Lorenzo Pieralisi [mailto:lorenzo.pieralisi@arm.com]
> > > > > > Sent: 22 September 2016 10:50
> > > > > > To: Bjorn Helgaas
> > > > > > Cc: Ard Biesheuvel; Tomasz Nowicki; David Daney; Will Deacon;
> Catalin
> > > > > > Marinas; Rafael Wysocki; Arnd Bergmann; Hanjun Guo; Sinan
> Kaya;
> > > > > > Jayachandran C; Christopher Covington; Duc Dang; Robert
> Richter; Marcin
> > > > > > Wojtas; Liviu Dudau; Wangyijing; Mark Salter; linux-
> > > > > > pci@vger.kernel.org; linux-arm-kernel@lists.infradead.org;
> Linaro ACPI
> > > > > > Mailman List; Jon Masters; Andrea Gallo; Jeremy Linton;
> liudongdong
> > > > > > (C); Gabriele Paoloni; Jeff Hugo; linux-acpi@vger.kernel.org;
> linux-
> > > > > > kernel@vger.kernel.org; Rafael J. Wysocki
> > > > > > Subject: Re: [PATCH V6 3/5] PCI: thunder-pem: Allow to probe
> PEM-
> > > > > > specific register range for ACPI case
> > > > > >
> > > > > > On Wed, Sep 21, 2016 at 01:04:57PM -0500, Bjorn Helgaas
> wrote:
> > > > > > > On Wed, Sep 21, 2016 at 03:05:49PM +0100, Lorenzo Pieralisi
> wrote:
> > > > > > > > On Tue, Sep 20, 2016 at 02:17:44PM -0500, Bjorn Helgaas
> wrote:
> > > > > > > > > On Tue, Sep 20, 2016 at 04:09:25PM +0100, Ard
> Biesheuvel wrote:
> > > > > > > >
> > > > > > > > [...]
> > > > > > > >
> > > > > > > > > > None of these platforms can be fixed entirely in
> software, and
> > > > > > given
> > > > > > > > > > that we will not be adding quirks for new broken
> hardware, we
> > > > > > should
> > > > > > > > > > ask ourselves whether having two versions of a quirk,
> i.e., one
> > > > > > for
> > > > > > > > > > broken hardware + currently shipping firmware, and
> one for the
> > > > > > same
> > > > > > > > > > broken hardware with fixed firmware is really an
> improvement
> > > > > > over what
> > > > > > > > > > has been proposed here.
> > > > > > > > >
> > > > > > > > > We're talking about two completely different types of
> quirks:
> > > > > > > > >
> > > > > > > > > 1) MCFG quirks to use memory-mapped config space that
> doesn't
> > > > > > quite
> > > > > > > > > conform to the ECAM model in the PCIe spec, and
> > > > > > > > >
> > > > > > > > > 2) Some yet-to-be-determined method to describe
> address space
> > > > > > > > > consumed by a bridge.
> > > > > > > > >
> > > > > > > > > The first two patches of this series are a nice
> implementation
> > > > > > for 1).
> > > > > > > > > The third patch (ThunderX-specific) is one possibility
> for 2),
> > > > > > but I
> > > > > > > > > don't like it because there's no way for generic
> software like
> > > > > > the
> > > > > > > > > ACPI core to discover these resources.
> > > > > > > >
> > > > > > > > Ok, so basically this means that to implement (2) we need
> to assign
> > > > > > > > some sort of _HID to these quirky PCI bridges (so that we
> know what
> > > > > > > > device they represent and we can retrieve their _CRS). I
> take from
> > > > > > > > this discussion that the goal is to make sure that all
> non-config
> > > > > > > > resources have to be declared through _CRS device
> objects, which is
> > > > > > > > fine but that requires a FW update (unless we can
> fabricate ACPI
> > > > > > > > devices and corresponding _CRS in the kernel whenever we
> match a
> > > > > > > > given MCFG table signature).
> > > > > > >
> > > > > > > All resources consumed by ACPI devices should be declared
> through
> > > > > > > _CRS. If you want to fabricate ACPI devices or _CRS via
> kernel
> > > > > > > quirks, that's fine with me. This could be triggered via
> MCFG
> > > > > > > signature, DMI info, host bridge _HID, etc.
> > > > > >
> > > > > > I think the PNP quirk approach + PNP0c02 resource put forward
> by Gab
> > > > > > is enough.
> > > > >
> > > > > Great thanks as we take a final decision I will ask Dogndgong
> to submit
> > > > > another RFC based on this approach
> > > > >
> > > > > >
> > > > > > > > We discussed this already and I think we should make a
> decision:
> > > > > > > >
> > > > > > > > http://lists.infradead.org/pipermail/linux-arm-
> kernel/2016-
> > > > > > March/414722.html
> > > > > > > >
> > > > > > > > > > > I'd like to step back and come up with some
> understanding of
> > > > > > how
> > > > > > > > > > > non-broken firmware *should* deal with this issue.
> Then, if
> > > > > > we *do*
> > > > > > > > > > > work around this particular broken firmware in the
> kernel, it
> > > > > > would be
> > > > > > > > > > > nice to do it in a way that fits in with that
> understanding.
> > > > > > > > > > >
> > > > > > > > > > > For example, if a companion ACPI device is the
> preferred
> > > > > > solution, an
> > > > > > > > > > > ACPI quirk could fabricate a device with the
> required
> > > > > > resources. That
> > > > > > > > > > > would address the problem closer to the source and
> make it
> > > > > > more likely
> > > > > > > > > > > that the rest of the system will work correctly:
> /proc/iomem
> > > > > > could
> > > > > > > > > > > make sense, things that look at _CRS generically
> would work
> > > > > > (e.g,
> > > > > > > > > > > /sys/, an admittedly hypothetical "lsacpi", etc.)
> > > > > > > > > > >
> > > > > > > > > > > Hard-coding stuff in drivers is a point solution
> that doesn't
> > > > > > provide
> > > > > > > > > > > any guidance for future platforms and makes it
> likely that
> > > > > > the hack
> > > > > > > > > > > will get copied into even more drivers.
> > > > > > > > > > >
> > > > > > > > > >
> > > > > > > > > > OK, I see. But the guidance for future platforms
> should be 'do
> > > > > > not
> > > > > > > > > > rely on quirks', and what I am arguing here is that
> the more we
> > > > > > polish
> > > > > > > > > > up this code and make it clean and reusable, the more
> likely it
> > > > > > is
> > > > > > > > > > that will end up getting abused by new broken
> hardware that we
> > > > > > set out
> > > > > > > > > > to reject entirely in the first place.
> > > > > > > > > >
> > > > > > > > > > So of course, if the quirk involves claiming
> resources, let's
> > > > > > make
> > > > > > > > > > sure that this occurs in the cleanest and most
> compliant way
> > > > > > possible.
> > > > > > > > > > But any factoring/reuse concerns other than for the
> current
> > > > > > crop of
> > > > > > > > > > broken hardware should be avoided imo.
> > > > > > > > >
> > > > > > > > > If future hardware is completely ECAM-compliant and we
> don't need
> > > > > > any
> > > > > > > > > more MCFG quirks, that would be great.
> > > > > > > >
> > > > > > > > Yes.
> > > > > > > >
> > > > > > > > > But we'll still need to describe that memory-mapped
> config space
> > > > > > > > > somewhere. If that's done with PNP0C02 or similar
> devices (as is
> > > > > > done
> > > > > > > > > on my x86 laptop), we'd be all set.
> > > > > > > >
> > > > > > > > I am not sure I understand what you mean here. Are you
> referring
> > > > > > > > to MCFG regions reported as PNP0c02 resources through its
> _CRS ?
> > > > > > >
> > > > > > > Yes. PCI Firmware Spec r3.0, Table 4-2, note 2 says
> address ranges
> > > > > > > reported via MCFG or _CBA should be reserved by _CRS of a
> PNP0C02
> > > > > > > device.
> > > > > >
> > > > > > Ok, that's agreed. It goes without saying that since you are
> quoting
> > > > > > the PCI spec, if FW fails to report MCFG regions in a PNP0c02
> device
> > > > > > _CRS I will consider that a FW bug.
> > > > > >
> > > > > > > > IIUC PNP0C02 is a reservation mechanism, but it does not
> help us
> > > > > > > > associate its _CRS to a specific PCI host bridge
> instance, right ?
> > > > > > >
> > > > > > > Gab proposed a hierarchy that *would* associate a PNP0C02
> device with
> > > > > > > a PCI bridge:
> > > > > > >
> > > > > > > Device (PCI1) {
> > > > > > > Name (_HID, "HISI0080") // PCI Express Root Bridge
> > > > > > > Name (_CID, "PNP0A03") // Compatible PCI Root Bridge
> > > > > > > Method (_CRS, 0, Serialized) { // Root complex
> resources
> > > > > > (windows) }
> > > > > > > Device (RES0) {
> > > > > > > Name (_HID, "HISI0081") // HiSi PCIe RC config base
> address
> > > > > > > Name (_CID, "PNP0C02") // Motherboard reserved
> resource
> > > > > > > Name (_CRS, ResourceTemplate () { ... }
> > > > > > > }
> > > > > > > }
> > > > > > >
> > > > > > > That's a possibility. The PCI Firmware Spec suggests
> putting RES0 at
> > > > > > > the root (under \_SB), but I don't know why.
> > > > > > >
> > > > > > > Putting it at the root means we couldn't generically
> associate it
> > > > > > with
> > > > > > > a bridge, although I could imagine something like this:
> > > > > > >
> > > > > > > Device (RES1) {
> > > > > > > Name (_HID, "HISI0081") // HiSi PCIe RC config base
> address
> > > > > > > Name (_CID, "PNP0C02") // Motherboard reserved
> resource
> > > > > > > Name (_CRS, ResourceTemplate () { ... }
> > > > > > > Method (BRDG) { "PCI1" } // hand-wavy ASL
> > > > > > > }
> > > > > > > Device (PCI1) {
> > > > > > > Name (_HID, "HISI0080") // PCI Express Root Bridge
> > > > > > > Name (_CID, "PNP0A03") // Compatible PCI Root Bridge
> > > > > > > Method (_CRS, 0, Serialized) { // Root complex
> resources
> > > > > > (windows) }
> > > > > > > }
> > > > > > >
> > > > > > > Where you could search PNP0C02 devices for a cookie that
> matched the
> > > > > > > host bridge.o
> > > > > >
> > > > > > Ok, I am fine with both and I think we are converging, but
> the way
> > > > > > to solve this problem has to be uniform for all ARM partners
> (and
> > > > > > not only ARM). Two points here:
> > > > > >
> > > > > > 1) Adding a device/subdevice allows people to add a _CRS
> reporting the
> > > > > > non-window bridge resources. Fine. It also allows people
> to chuck in
> > > > > > there all sorts of _DSD properties to describe their PCI
> host bridge
> > > > > > as it is done with DT properties (those _DSD can contain
> eg clocks
> > > > > > etc.), this may be tempting (so that they can reuse the
> same DT
> > > > > > driver and do not have to update their firmware) but I
> want to be
> > > > > > clear here: that must not happen. So, a subdevice with a
> _CRS to
> > > > > > report resources, yes, but it will stop there.
> > > > > > 2) It is unclear to me how to formalize the above. People
> should not
> > > > > > write FW by reading the PCI mailing list, so these
> guidelines have
> > > > > > to
> > > > > > be written, somehow. I do not want to standardize quirks,
> I want
> > > > > > to prevent random ACPI table content, which is different.
> > > > > > Should I report this to the ACPI spec working group ? If
> we do
> > > > > > not do that everyone will go solve this problem as they
> deem fit.
> > > > > >
> > > > >
> > > > > Do we really need to formalize this?
> > > > >
> > > > > As we discussed in the Linaro call at the moment we have few
> vendors
> > > > > that need quirks and we want to avoid promoting/accepting
> quirks for
> > > > > the future.
> > > > >
> > > > > At the time of the call I think we decided to informally accept
> a set
> > > > > of quirks for the current platforms and reject any other quirk
> coming
> > > > > after a certain date/kernel version (this to be decided).
> > > > >
> > > > > I am not sure if there is a way to document/formalize a
> temporary
> > > > > exception from the rule...
> > > >
> > > > - (1) will be enforced.
> > >
> > > I'm not sure it's necessary or possible to enforce a "no future
> > > quirks" rule. For one thing, there's already a pretty strong
> > > incentive to avoid quirks: if your hardware doesn't require quirks,
> > > it works with OSes already in the field.
> > >
> > > MCFG quirks allow us to use the generic ACPI pci_root.c driver even
> if
> > > the hardware doesn't support ECAM quite according to the spec.
> > >
> > > PNP0C02 usage is a workaround for the failure of the
> Consumer/Producer
> > > bit. PNP0C02 quirks compensate for firmware that doesn't describe
> > > resource usage accurately. It's possible the ACPI spec folks could
> > > come up with a better Consumer/Producer workaround, if that's
> needed.
> > > Apparently x86 hasn't needed it yet.
> > >
> > > If people add _DSD methods for clocks or whatnot, the hardware
> won't
> > > work with the generic pci_root.c driver, so there's already an
> > > incentive for avoiding them. x86 has managed without such methods;
> > > arm64 should be able to do the same.
> >
> > Re-reading this, I'm afraid my response sounds a little dismissive,
> > and I feel like I'm missing some important information. So I
> > apologize if I missed your whole point, Lorenzo.
>
> No you are spot on, I just wanted to emphasize, given that we are
> adding an _HID and a subdevice, that developer should not be tempted
> to use it to match against a PCI host driver to reuse the DT code,
> we should not use the quirk mechanism as a backdoor to re-using DT
> drivers in ACPI context.
>
> Anyway, there is a review process to spot these possible misuses,
> mine was just a heads-up, quirks will happen, I just do not want
> to wreak the standard ACPI PCI firmware model to support them.
>
> Given that there are already PNP0c02 bindings out there where the
> PNP0c02 is used as in Gab's example:
>
> https://patchwork.kernel.org/patch/4757111/
>
> I think the only pending question I have is whether we are allowed
> to define a PNP0A03 subdevice with a _CRS resource space that is
> not contained in its parent _CRS, if we answer this question I
> think we are done.
FMU part of your question is answered in the PCI Firmware specs
https://members.pcisig.com/wg/PCI-SIG/document/download/8232
Where from note 2 of 4.1.2 I quote:
"For most systems, the motherboard resource would appear at the root
of the ACPI namespace (under \_SB) in a node with a _HID of EISAID
(PNP0C02), and the resources in this case should not be claimed in the
root PCI bus's _CRS"
My interpretation is that the resource claimed in the PNP0C02 node
must never be in the PNP0A03 _CRS.
Now about having the PNP0C02 node under \_SB or as a sub-device we see
that the note above points out that most of system have it under \_SB
but I read it as a quite relaxed condition....
BTW this is just my interpretation...
Thanks
Gab
>
> I will raise the PNP0c02 usage issue with the ASWG anyway.
>
> Thanks !
> Lorenzo
[toc] | [prev] | [next] | [standalone]
| From | Christopher Covington <cov@codeaurora.org> |
|---|---|
| Date | 2016-09-22 16:30 +0200 |
| Subject | Re: [PATCH V6 3/5] PCI: thunder-pem: Allow to probe PEM-specific register range for ACPI case |
| Message-ID | <skdaG-7wd-19@gated-at.bofh.it> |
| In reply to | #1488699 |
On 09/22/2016 05:49 AM, Lorenzo Pieralisi wrote:
> On Wed, Sep 21, 2016 at 01:04:57PM -0500, Bjorn Helgaas wrote:
>> On Wed, Sep 21, 2016 at 03:05:49PM +0100, Lorenzo Pieralisi wrote:
>>> On Tue, Sep 20, 2016 at 02:17:44PM -0500, Bjorn Helgaas wrote:
>>>> On Tue, Sep 20, 2016 at 04:09:25PM +0100, Ard Biesheuvel wrote:
>>>
>>> [...]
>>>
>>>>> None of these platforms can be fixed entirely in software, and given
>>>>> that we will not be adding quirks for new broken hardware, we should
>>>>> ask ourselves whether having two versions of a quirk, i.e., one for
>>>>> broken hardware + currently shipping firmware, and one for the same
>>>>> broken hardware with fixed firmware is really an improvement over what
>>>>> has been proposed here.
>>>>
>>>> We're talking about two completely different types of quirks:
>>>>
>>>> 1) MCFG quirks to use memory-mapped config space that doesn't quite
>>>> conform to the ECAM model in the PCIe spec, and
>>>>
>>>> 2) Some yet-to-be-determined method to describe address space
>>>> consumed by a bridge.
>>>>
>>>> The first two patches of this series are a nice implementation for 1).
>>>> The third patch (ThunderX-specific) is one possibility for 2), but I
>>>> don't like it because there's no way for generic software like the
>>>> ACPI core to discover these resources.
>>>
>>> Ok, so basically this means that to implement (2) we need to assign
>>> some sort of _HID to these quirky PCI bridges (so that we know what
>>> device they represent and we can retrieve their _CRS). I take from
>>> this discussion that the goal is to make sure that all non-config
>>> resources have to be declared through _CRS device objects, which is
>>> fine but that requires a FW update (unless we can fabricate ACPI
>>> devices and corresponding _CRS in the kernel whenever we match a
>>> given MCFG table signature).
>>
>> All resources consumed by ACPI devices should be declared through
>> _CRS. If you want to fabricate ACPI devices or _CRS via kernel
>> quirks, that's fine with me. This could be triggered via MCFG
>> signature, DMI info, host bridge _HID, etc.
>
> I think the PNP quirk approach + PNP0c02 resource put forward by Gab
> is enough.
>
>>> We discussed this already and I think we should make a decision:
>>>
>>> http://lists.infradead.org/pipermail/linux-arm-kernel/2016-March/414722.html
>>>
>>>>>> I'd like to step back and come up with some understanding of how
>>>>>> non-broken firmware *should* deal with this issue. Then, if we *do*
>>>>>> work around this particular broken firmware in the kernel, it would be
>>>>>> nice to do it in a way that fits in with that understanding.
>>>>>>
>>>>>> For example, if a companion ACPI device is the preferred solution, an
>>>>>> ACPI quirk could fabricate a device with the required resources. That
>>>>>> would address the problem closer to the source and make it more likely
>>>>>> that the rest of the system will work correctly: /proc/iomem could
>>>>>> make sense, things that look at _CRS generically would work (e.g,
>>>>>> /sys/, an admittedly hypothetical "lsacpi", etc.)
>>>>>>
>>>>>> Hard-coding stuff in drivers is a point solution that doesn't provide
>>>>>> any guidance for future platforms and makes it likely that the hack
>>>>>> will get copied into even more drivers.
>>>>>>
>>>>>
>>>>> OK, I see. But the guidance for future platforms should be 'do not
>>>>> rely on quirks', and what I am arguing here is that the more we polish
>>>>> up this code and make it clean and reusable, the more likely it is
>>>>> that will end up getting abused by new broken hardware that we set out
>>>>> to reject entirely in the first place.
>>>>>
>>>>> So of course, if the quirk involves claiming resources, let's make
>>>>> sure that this occurs in the cleanest and most compliant way possible.
>>>>> But any factoring/reuse concerns other than for the current crop of
>>>>> broken hardware should be avoided imo.
>>>>
>>>> If future hardware is completely ECAM-compliant and we don't need any
>>>> more MCFG quirks, that would be great.
>>>
>>> Yes.
>>>
>>>> But we'll still need to describe that memory-mapped config space
>>>> somewhere. If that's done with PNP0C02 or similar devices (as is done
>>>> on my x86 laptop), we'd be all set.
>>>
>>> I am not sure I understand what you mean here. Are you referring
>>> to MCFG regions reported as PNP0c02 resources through its _CRS ?
>>
>> Yes. PCI Firmware Spec r3.0, Table 4-2, note 2 says address ranges
>> reported via MCFG or _CBA should be reserved by _CRS of a PNP0C02
>> device.
>
> Ok, that's agreed. It goes without saying that since you are quoting
> the PCI spec, if FW fails to report MCFG regions in a PNP0c02 device
> _CRS I will consider that a FW bug.
>
>>> IIUC PNP0C02 is a reservation mechanism, but it does not help us
>>> associate its _CRS to a specific PCI host bridge instance, right ?
>>
>> Gab proposed a hierarchy that *would* associate a PNP0C02 device with
>> a PCI bridge:
>>
>> Device (PCI1) {
>> Name (_HID, "HISI0080") // PCI Express Root Bridge
>> Name (_CID, "PNP0A03") // Compatible PCI Root Bridge
>> Method (_CRS, 0, Serialized) { // Root complex resources (windows) }
>> Device (RES0) {
>> Name (_HID, "HISI0081") // HiSi PCIe RC config base address
>> Name (_CID, "PNP0C02") // Motherboard reserved resource
>> Name (_CRS, ResourceTemplate () { ... }
>> }
>> }
>>
>> That's a possibility. The PCI Firmware Spec suggests putting RES0 at
>> the root (under \_SB), but I don't know why.
>>
>> Putting it at the root means we couldn't generically associate it with
>> a bridge, although I could imagine something like this:
>>
>> Device (RES1) {
>> Name (_HID, "HISI0081") // HiSi PCIe RC config base address
>> Name (_CID, "PNP0C02") // Motherboard reserved resource
>> Name (_CRS, ResourceTemplate () { ... }
>> Method (BRDG) { "PCI1" } // hand-wavy ASL
>> }
>> Device (PCI1) {
>> Name (_HID, "HISI0080") // PCI Express Root Bridge
>> Name (_CID, "PNP0A03") // Compatible PCI Root Bridge
>> Method (_CRS, 0, Serialized) { // Root complex resources (windows) }
>> }
>>
>> Where you could search PNP0C02 devices for a cookie that matched the
>> host bridge.o
>
> Ok, I am fine with both and I think we are converging, but the way
> to solve this problem has to be uniform for all ARM partners (and
> not only ARM). Two points here:
>
> 1) Adding a device/subdevice allows people to add a _CRS reporting the
> non-window bridge resources. Fine. It also allows people to chuck in
> there all sorts of _DSD properties to describe their PCI host bridge
> as it is done with DT properties (those _DSD can contain eg clocks
> etc.), this may be tempting (so that they can reuse the same DT
> driver and do not have to update their firmware) but I want to be
> clear here: that must not happen. So, a subdevice with a _CRS to
> report resources, yes, but it will stop there.
> 2) It is unclear to me how to formalize the above. People should not
> write FW by reading the PCI mailing list, so these guidelines have to
> be written, somehow. I do not want to standardize quirks, I want
> to prevent random ACPI table content, which is different.
> Should I report this to the ACPI spec working group ? If we do
> not do that everyone will go solve this problem as they deem fit.
Could you add some checks to fwts?
Cov
--
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm
Technologies, Inc. Qualcomm Technologies, Inc. is a member of the Code
Aurora Forum, a Linux Foundation Collaborative Project.
[toc] | [prev] | [next] | [standalone]
| From | Gabriele Paoloni <gabriele.paoloni@huawei.com> |
|---|---|
| Date | 2016-09-21 16:20 +0200 |
| Subject | RE: [PATCH V6 3/5] PCI: thunder-pem: Allow to probe PEM-specific register range for ACPI case |
| Message-ID | <sjQxs-1Ea-3@gated-at.bofh.it> |
| In reply to | #1487566 |
Hi Bjorn [...] > > If future hardware is completely ECAM-compliant and we don't need any > more MCFG quirks, that would be great. > > But we'll still need to describe that memory-mapped config space > somewhere. If that's done with PNP0C02 or similar devices (as is done > on my x86 laptop), we'd be all set. > > If we need to work around firmware in the field that doesn't do that, > one possibility is a PNP quirk along the lines of > quirk_amd_mmconfig_area(). So, if my understanding is correct, for platforms that have not been shipped yet you propose to use PNP0C02 in the ACPI table in order to declare a motherboard reserved resource whereas for shipped platforms you propose to have a quirk along pnp_fixups in order to track the resource usage even if values are hardcoded...correct? Before Tomasz came up with this patchset we had a call between the vendors involved in this PCI quirks saga and other guys from Linaro and ARM. Lorenzo summarized the outcome as in the following link http://lkml.iu.edu/hypermail/linux/kernel/1606.2/03344.html Since this quirks mechanism has been discussed for quite a long time now IMHO it would be good to have a last call including also you (Bjorn) so that we can all agree on what to do and we avoid changing our drivers again and again... What do you think? Thanks Gab > > Bjorn
[toc] | [prev] | [next] | [standalone]
| From | Bjorn Helgaas <helgaas@kernel.org> |
|---|---|
| Date | 2016-09-21 21:00 +0200 |
| Subject | Re: [PATCH V6 3/5] PCI: thunder-pem: Allow to probe PEM-specific register range for ACPI case |
| Message-ID | <sjUUp-4cs-13@gated-at.bofh.it> |
| In reply to | #1488149 |
On Wed, Sep 21, 2016 at 02:10:55PM +0000, Gabriele Paoloni wrote: > Hi Bjorn > > [...] > > > > > > If future hardware is completely ECAM-compliant and we don't need any > > more MCFG quirks, that would be great. > > > > But we'll still need to describe that memory-mapped config space > > somewhere. If that's done with PNP0C02 or similar devices (as is done > > on my x86 laptop), we'd be all set. > > > > If we need to work around firmware in the field that doesn't do that, > > one possibility is a PNP quirk along the lines of > > quirk_amd_mmconfig_area(). > > So, if my understanding is correct, for platforms that have not been > shipped yet you propose to use PNP0C02 in the ACPI table in order to > declare a motherboard reserved resource whereas for shipped platforms > you propose to have a quirk along pnp_fixups in order to track the > resource usage even if values are hardcoded...correct? Yes. I'm open to alternate proposals, but x86 uses PNP0C02, and following existing practice seems reasonable. > Before Tomasz came up with this patchset we had a call between the vendors > involved in this PCI quirks saga and other guys from Linaro and ARM. > > Lorenzo summarized the outcome as in the following link > http://lkml.iu.edu/hypermail/linux/kernel/1606.2/03344.html > > Since this quirks mechanism has been discussed for quite a long time now > IMHO it would be good to have a last call including also you (Bjorn) so > that we can all agree on what to do and we avoid changing our drivers again > and again... I think we're converging pretty fast. As far as I'm concerned, the v6 ECAM quirks implementation is perfect. The only remaining issue is reporting the ECAM resources, and I haven't seen objections to using PNP0C02 + PNP quirks for broken firmware. There is the question of how or whether to associate a PNP0A03 PCI bridge with resources from a different PNP0C02 device, but that's not super important. If the hard-coded resources appear both in a quirk and in the PCI bridge driver, it's ugly but not the end of the world. We've still achieved the objective of avoiding landmines in the address space. Bjorn
[toc] | [prev] | [next] | [standalone]
| From | Gabriele Paoloni <gabriele.paoloni@huawei.com> |
|---|---|
| Date | 2016-09-22 13:20 +0200 |
| Subject | RE: [PATCH V6 3/5] PCI: thunder-pem: Allow to probe PEM-specific register range for ACPI case |
| Message-ID | <skacN-5BK-1@gated-at.bofh.it> |
| In reply to | #1488321 |
Hi Bjorn > -----Original Message----- > From: Bjorn Helgaas [mailto:helgaas@kernel.org] > Sent: 21 September 2016 19:59 > To: Gabriele Paoloni > Cc: Ard Biesheuvel; Tomasz Nowicki; David Daney; Will Deacon; Catalin > Marinas; Rafael Wysocki; Lorenzo Pieralisi; Arnd Bergmann; Hanjun Guo; > Sinan Kaya; Jayachandran C; Christopher Covington; Duc Dang; Robert > Richter; Marcin Wojtas; Liviu Dudau; Wangyijing; Mark Salter; linux- > pci@vger.kernel.org; linux-arm-kernel@lists.infradead.org; Linaro ACPI > Mailman List; Jon Masters; Andrea Gallo; Jeremy Linton; liudongdong > (C); Jeff Hugo; linux-acpi@vger.kernel.org; linux- > kernel@vger.kernel.org; Rafael J. Wysocki > Subject: Re: [PATCH V6 3/5] PCI: thunder-pem: Allow to probe PEM- > specific register range for ACPI case > > On Wed, Sep 21, 2016 at 02:10:55PM +0000, Gabriele Paoloni wrote: > > Hi Bjorn > > > > [...] > > > > > > > > > > If future hardware is completely ECAM-compliant and we don't need > any > > > more MCFG quirks, that would be great. > > > > > > But we'll still need to describe that memory-mapped config space > > > somewhere. If that's done with PNP0C02 or similar devices (as is > done > > > on my x86 laptop), we'd be all set. > > > > > > If we need to work around firmware in the field that doesn't do > that, > > > one possibility is a PNP quirk along the lines of > > > quirk_amd_mmconfig_area(). > > > > So, if my understanding is correct, for platforms that have not been > > shipped yet you propose to use PNP0C02 in the ACPI table in order to > > declare a motherboard reserved resource whereas for shipped platforms > > you propose to have a quirk along pnp_fixups in order to track the > > resource usage even if values are hardcoded...correct? > > Yes. I'm open to alternate proposals, but x86 uses PNP0C02, and > following existing practice seems reasonable. > > > Before Tomasz came up with this patchset we had a call between the > vendors > > involved in this PCI quirks saga and other guys from Linaro and ARM. > > > > Lorenzo summarized the outcome as in the following link > > http://lkml.iu.edu/hypermail/linux/kernel/1606.2/03344.html > > > > Since this quirks mechanism has been discussed for quite a long time > now > > IMHO it would be good to have a last call including also you (Bjorn) > so > > that we can all agree on what to do and we avoid changing our drivers > again > > and again... > > I think we're converging pretty fast. As far as I'm concerned, the > v6 ECAM quirks implementation is perfect. The only remaining issue is > reporting the ECAM resources, and I haven't seen objections to using > PNP0C02 + PNP quirks for broken firmware. > > There is the question of how or whether to associate a PNP0A03 PCI > bridge with resources from a different PNP0C02 device, but that's not > super important. If the hard-coded resources appear both in a quirk > and in the PCI bridge driver, it's ugly but not the end of the world. > We've still achieved the objective of avoiding landmines in the > address space. Ok got it many thanks Gab > > Bjorn
[toc] | [prev] | [next] | [standalone]
| From | Tomasz Nowicki <tn@semihalf.com> |
|---|---|
| Date | 2016-09-09 21:30 +0200 |
| Subject | [PATCH V6 1/5] PCI/ACPI: Extend pci_mcfg_lookup() responsibilities |
| Message-ID | <sfzES-6Yg-25@gated-at.bofh.it> |
| In reply to | #1480276 |
In preparation for adding MCFG platform specific quirk handling move
CFG resource calculation and ECAM ops assignment to pci_mcfg_lookup().
It becomes the gate for further ops and CFG resource manipulation
in arch-agnostic code (drivers/acpi/pci_mcfg.c).
No functionality changes in this patch.
Signed-off-by: Tomasz Nowicki <tn@semihalf.com>
---
arch/arm64/kernel/pci.c | 17 +++++------------
drivers/acpi/pci_mcfg.c | 28 +++++++++++++++++++++++++---
include/linux/pci-acpi.h | 4 +++-
3 files changed, 33 insertions(+), 16 deletions(-)
diff --git a/arch/arm64/kernel/pci.c b/arch/arm64/kernel/pci.c
index acf3872..fb439c7 100644
--- a/arch/arm64/kernel/pci.c
+++ b/arch/arm64/kernel/pci.c
@@ -125,24 +125,17 @@ pci_acpi_setup_ecam_mapping(struct acpi_pci_root *root)
u16 seg = root->segment;
struct pci_config_window *cfg;
struct resource cfgres;
- unsigned int bsz;
+ struct pci_ecam_ops *ecam_ops;
+ int ret;
- /* Use address from _CBA if present, otherwise lookup MCFG */
- if (!root->mcfg_addr)
- root->mcfg_addr = pci_mcfg_lookup(seg, bus_res);
-
- if (!root->mcfg_addr) {
+ ret = pci_mcfg_lookup(root, &cfgres, &ecam_ops);
+ if (ret) {
dev_err(&root->device->dev, "%04x:%pR ECAM region not found\n",
seg, bus_res);
return NULL;
}
- bsz = 1 << pci_generic_ecam_ops.bus_shift;
- cfgres.start = root->mcfg_addr + bus_res->start * bsz;
- cfgres.end = cfgres.start + resource_size(bus_res) * bsz - 1;
- cfgres.flags = IORESOURCE_MEM;
- cfg = pci_ecam_create(&root->device->dev, &cfgres, bus_res,
- &pci_generic_ecam_ops);
+ cfg = pci_ecam_create(&root->device->dev, &cfgres, bus_res, ecam_ops);
if (IS_ERR(cfg)) {
dev_err(&root->device->dev, "%04x:%pR error %ld mapping ECAM\n",
seg, bus_res, PTR_ERR(cfg));
diff --git a/drivers/acpi/pci_mcfg.c b/drivers/acpi/pci_mcfg.c
index b5b376e..ffcc651 100644
--- a/drivers/acpi/pci_mcfg.c
+++ b/drivers/acpi/pci_mcfg.c
@@ -22,6 +22,7 @@
#include <linux/kernel.h>
#include <linux/pci.h>
#include <linux/pci-acpi.h>
+#include <linux/pci-ecam.h>
/* Structure to hold entries from the MCFG table */
struct mcfg_entry {
@@ -35,9 +36,18 @@ struct mcfg_entry {
/* List to save MCFG entries */
static LIST_HEAD(pci_mcfg_list);
-phys_addr_t pci_mcfg_lookup(u16 seg, struct resource *bus_res)
+int pci_mcfg_lookup(struct acpi_pci_root *root, struct resource *cfgres,
+ struct pci_ecam_ops **ecam_ops)
{
+ struct pci_ecam_ops *ops = &pci_generic_ecam_ops;
+ struct resource *bus_res = &root->secondary;
+ u16 seg = root->segment;
struct mcfg_entry *e;
+ struct resource res;
+
+ /* Use address from _CBA if present, otherwise lookup MCFG */
+ if (root->mcfg_addr)
+ goto skip_lookup;
/*
* We expect exact match, unless MCFG entry end bus covers more than
@@ -45,10 +55,22 @@ phys_addr_t pci_mcfg_lookup(u16 seg, struct resource *bus_res)
*/
list_for_each_entry(e, &pci_mcfg_list, list) {
if (e->segment == seg && e->bus_start == bus_res->start &&
- e->bus_end >= bus_res->end)
- return e->addr;
+ e->bus_end >= bus_res->end) {
+ root->mcfg_addr = e->addr;
+ }
+
}
+ if (!root->mcfg_addr)
+ return -ENXIO;
+
+skip_lookup:
+ memset(&res, 0, sizeof(res));
+ res.start = root->mcfg_addr + (bus_res->start << 20);
+ res.end = res.start + (resource_size(bus_res) << 20) - 1;
+ res.flags = IORESOURCE_MEM;
+ *cfgres = res;
+ *ecam_ops = ops;
return 0;
}
diff --git a/include/linux/pci-acpi.h b/include/linux/pci-acpi.h
index 7d63a66..7a4e83a 100644
--- a/include/linux/pci-acpi.h
+++ b/include/linux/pci-acpi.h
@@ -24,7 +24,9 @@ static inline acpi_status pci_acpi_remove_pm_notifier(struct acpi_device *dev)
}
extern phys_addr_t acpi_pci_root_get_mcfg_addr(acpi_handle handle);
-extern phys_addr_t pci_mcfg_lookup(u16 domain, struct resource *bus_res);
+struct pci_ecam_ops;
+extern int pci_mcfg_lookup(struct acpi_pci_root *root, struct resource *cfgres,
+ struct pci_ecam_ops **ecam_ops);
static inline acpi_handle acpi_find_root_bridge_handle(struct pci_dev *pdev)
{
--
1.9.1
[toc] | [prev] | [next] | [standalone]
| From | Tomasz Nowicki <tn@semihalf.com> |
|---|---|
| Date | 2016-09-09 21:40 +0200 |
| Message-ID | <sfzOy-71n-15@gated-at.bofh.it> |
| In reply to | #1480276 |
On 09.09.2016 21:24, Tomasz Nowicki wrote: > Quirk handling relies on an idea of simple static array which contains > quirk enties. Each entry consists of identification information (IDs from > standard header of MCFG table) along with custom pci_ecam_ops structure and > configuration space resource structure. This way it is possible find > corresponding quirk entries and override pci_ecam_ops and PCI configuration > space regions. > > As an example, the last 3 patches present quirk handling mechanism usage for > ThunderX. > This series can be found here: git@github.com:semihalf-nowicki-tomasz/linux.git (branch: pci-acpi-quirk-v6) Thanks, Tomasz
[toc] | [prev] | [next] | [standalone]
| From | Bjorn Helgaas <helgaas@kernel.org> |
|---|---|
| Date | 2016-09-20 21:30 +0200 |
| Message-ID | <sjyTU-7bb-21@gated-at.bofh.it> |
| In reply to | #1480276 |
On Fri, Sep 09, 2016 at 09:24:02PM +0200, Tomasz Nowicki wrote:
> Quirk handling relies on an idea of simple static array which contains
> quirk enties. Each entry consists of identification information (IDs from
> standard header of MCFG table) along with custom pci_ecam_ops structure and
> configuration space resource structure. This way it is possible find
> corresponding quirk entries and override pci_ecam_ops and PCI configuration
> space regions.
>
> As an example, the last 3 patches present quirk handling mechanism usage for
> ThunderX.
>
> v5 -> v6
> - rebase against v4.8-rc5
> - drop patch 1 form previous series
> - keep pci_acpi_setup_ecam_mapping() in ARM64 arch directory
> - move quirk code to pci_mcfg.c
> - restrict quirk to override pci_ecam_ops and CFG resource structure
> only, no init call any more
> - split ThunderX quirks into the smaller chunks
> - add ThunderX pass1.x silicon revision support
>
> v4 -> v5
> - rebase against v4.8-rc1
> - rework to exact MCFG OEM ID, TABLE ID, rev match
> - use memcmp instead of strncmp
> - no substring match
> - fix typos and dmesg message
>
> Tomasz Nowicki (5):
> PCI/ACPI: Extend pci_mcfg_lookup() responsibilities
> PCI/ACPI: Check platform specific ECAM quirks
> PCI: thunder-pem: Allow to probe PEM-specific register range for ACPI
> case
> PCI: thunder: Enable ACPI PCI controller for ThunderX pass2.x silicon
> version
> PCI: thunder: Enable ACPI PCI controller for ThunderX pass1.x silicon
> version
>
> arch/arm64/kernel/pci.c | 17 ++--
> drivers/acpi/pci_mcfg.c | 168 +++++++++++++++++++++++++++++++++++-
> drivers/pci/host/pci-thunder-ecam.c | 2 +-
> drivers/pci/host/pci-thunder-pem.c | 63 +++++++++++---
> include/linux/pci-acpi.h | 4 +-
> include/linux/pci-ecam.h | 7 ++
> 6 files changed, 230 insertions(+), 31 deletions(-)
I'm not quite ready to merge these because we haven't resolved the
question of how to expose the resources used by the memory-mapped
config space. I'm fine with the first two patches (I did make a
couple trivial changes, see below), but there's no point in merging
them until we merge a user for them.
I pushed the series to pci/ecam-v6 for build testing and discussion.
The diff (the changes I made locally) from v6 as posted by Tomasz is
below.
Bjorn
diff --git a/drivers/acpi/pci_mcfg.c b/drivers/acpi/pci_mcfg.c
index eb14f74..bb3b8ad 100644
--- a/drivers/acpi/pci_mcfg.c
+++ b/drivers/acpi/pci_mcfg.c
@@ -42,86 +42,59 @@ struct mcfg_fixup {
struct resource cfgres;
};
-#define MCFG_DOM_ANY (-1)
#define MCFG_BUS_RANGE(start, end) DEFINE_RES_NAMED((start), \
((end) - (start) + 1), \
NULL, IORESOURCE_BUS)
-#define MCFG_BUS_ANY MCFG_BUS_RANGE(0x0, 0xff)
-#define MCFG_RES_EMPTY DEFINE_RES_NAMED(0, 0, NULL, 0)
+#define MCFG_BUS_ANY MCFG_BUS_RANGE(0x0, 0xff)
static struct mcfg_fixup mcfg_quirks[] = {
-/* { OEM_ID, OEM_TABLE_ID, REV, DOMAIN, BUS_RANGE, cfgres, ops }, */
+/* { OEM_ID, OEM_TABLE_ID, REV, SEGMENT, BUS_RANGE, cfgres, ops }, */
#ifdef CONFIG_PCI_HOST_THUNDER_PEM
+#define THUNDER_PEM_MCFG(rev, seg, addr) \
+ { "CAVIUM", "THUNDERX", rev, seg, MCFG_BUS_ANY, \
+ &pci_thunder_pem_ops, DEFINE_RES_MEM(addr, 0x39 * SZ_16M) }
+
/* SoC pass2.x */
- { "CAVIUM", "THUNDERX", 1, 4, MCFG_BUS_ANY, &pci_thunder_pem_ops,
- DEFINE_RES_MEM(0x88001f000000UL, 0x39 * SZ_16M) },
- { "CAVIUM", "THUNDERX", 1, 5, MCFG_BUS_ANY, &pci_thunder_pem_ops,
- DEFINE_RES_MEM(0x884057000000UL, 0x39 * SZ_16M) },
- { "CAVIUM", "THUNDERX", 1, 6, MCFG_BUS_ANY, &pci_thunder_pem_ops,
- DEFINE_RES_MEM(0x88808f000000UL, 0x39 * SZ_16M) },
- { "CAVIUM", "THUNDERX", 1, 7, MCFG_BUS_ANY, &pci_thunder_pem_ops,
- DEFINE_RES_MEM(0x89001f000000UL, 0x39 * SZ_16M) },
- { "CAVIUM", "THUNDERX", 1, 8, MCFG_BUS_ANY, &pci_thunder_pem_ops,
- DEFINE_RES_MEM(0x894057000000UL, 0x39 * SZ_16M) },
- { "CAVIUM", "THUNDERX", 1, 9, MCFG_BUS_ANY, &pci_thunder_pem_ops,
- DEFINE_RES_MEM(0x89808f000000UL, 0x39 * SZ_16M) },
- { "CAVIUM", "THUNDERX", 1, 14, MCFG_BUS_ANY, &pci_thunder_pem_ops,
- DEFINE_RES_MEM(0x98001f000000UL, 0x39 * SZ_16M) },
- { "CAVIUM", "THUNDERX", 1, 15, MCFG_BUS_ANY, &pci_thunder_pem_ops,
- DEFINE_RES_MEM(0x984057000000UL, 0x39 * SZ_16M) },
- { "CAVIUM", "THUNDERX", 1, 16, MCFG_BUS_ANY, &pci_thunder_pem_ops,
- DEFINE_RES_MEM(0x98808f000000UL, 0x39 * SZ_16M) },
- { "CAVIUM", "THUNDERX", 1, 17, MCFG_BUS_ANY, &pci_thunder_pem_ops,
- DEFINE_RES_MEM(0x99001f000000UL, 0x39 * SZ_16M) },
- { "CAVIUM", "THUNDERX", 1, 18, MCFG_BUS_ANY, &pci_thunder_pem_ops,
- DEFINE_RES_MEM(0x994057000000UL, 0x39 * SZ_16M) },
- { "CAVIUM", "THUNDERX", 1, 19, MCFG_BUS_ANY, &pci_thunder_pem_ops,
- DEFINE_RES_MEM(0x99808f000000UL, 0x39 * SZ_16M) },
+ THUNDER_PEM_MCFG(1, 4, 0x88001f000000UL),
+ THUNDER_PEM_MCFG(1, 5, 0x884057000000UL),
+ THUNDER_PEM_MCFG(1, 6, 0x88808f000000UL),
+ THUNDER_PEM_MCFG(1, 7, 0x89001f000000UL),
+ THUNDER_PEM_MCFG(1, 8, 0x894057000000UL),
+ THUNDER_PEM_MCFG(1, 9, 0x89808f000000UL),
+ THUNDER_PEM_MCFG(1, 14, 0x98001f000000UL),
+ THUNDER_PEM_MCFG(1, 15, 0x984057000000UL),
+ THUNDER_PEM_MCFG(1, 16, 0x98808f000000UL),
+ THUNDER_PEM_MCFG(1, 17, 0x99001f000000UL),
+ THUNDER_PEM_MCFG(1, 18, 0x994057000000UL),
+ THUNDER_PEM_MCFG(1, 19, 0x99808f000000UL),
/* SoC pass1.x */
- { "CAVIUM", "THUNDERX", 2, 4, MCFG_BUS_ANY, &pci_thunder_pem_ops,
- DEFINE_RES_MEM(0x88001f000000UL, 0x39 * SZ_16M) },
- { "CAVIUM", "THUNDERX", 2, 5, MCFG_BUS_ANY, &pci_thunder_pem_ops,
- DEFINE_RES_MEM(0x884057000000UL, 0x39 * SZ_16M) },
- { "CAVIUM", "THUNDERX", 2, 6, MCFG_BUS_ANY, &pci_thunder_pem_ops,
- DEFINE_RES_MEM(0x88808f000000UL, 0x39 * SZ_16M) },
- { "CAVIUM", "THUNDERX", 2, 7, MCFG_BUS_ANY, &pci_thunder_pem_ops,
- DEFINE_RES_MEM(0x89001f000000UL, 0x39 * SZ_16M) },
- { "CAVIUM", "THUNDERX", 2, 8, MCFG_BUS_ANY, &pci_thunder_pem_ops,
- DEFINE_RES_MEM(0x894057000000UL, 0x39 * SZ_16M) },
- { "CAVIUM", "THUNDERX", 2, 9, MCFG_BUS_ANY, &pci_thunder_pem_ops,
- DEFINE_RES_MEM(0x89808f000000UL, 0x39 * SZ_16M) },
- { "CAVIUM", "THUNDERX", 2, 14, MCFG_BUS_ANY, &pci_thunder_pem_ops,
- DEFINE_RES_MEM(0x98001f000000UL, 0x39 * SZ_16M) },
- { "CAVIUM", "THUNDERX", 2, 15, MCFG_BUS_ANY, &pci_thunder_pem_ops,
- DEFINE_RES_MEM(0x984057000000UL, 0x39 * SZ_16M) },
- { "CAVIUM", "THUNDERX", 2, 16, MCFG_BUS_ANY, &pci_thunder_pem_ops,
- DEFINE_RES_MEM(0x98808f000000UL, 0x39 * SZ_16M) },
- { "CAVIUM", "THUNDERX", 2, 17, MCFG_BUS_ANY, &pci_thunder_pem_ops,
- DEFINE_RES_MEM(0x99001f000000UL, 0x39 * SZ_16M) },
- { "CAVIUM", "THUNDERX", 2, 18, MCFG_BUS_ANY, &pci_thunder_pem_ops,
- DEFINE_RES_MEM(0x994057000000UL, 0x39 * SZ_16M) },
- { "CAVIUM", "THUNDERX", 2, 19, MCFG_BUS_ANY, &pci_thunder_pem_ops,
- DEFINE_RES_MEM(0x99808f000000UL, 0x39 * SZ_16M) },
+ THUNDER_PEM_MCFG(2, 4, 0x88001f000000UL),
+ THUNDER_PEM_MCFG(2, 5, 0x884057000000UL),
+ THUNDER_PEM_MCFG(2, 6, 0x88808f000000UL),
+ THUNDER_PEM_MCFG(2, 7, 0x89001f000000UL),
+ THUNDER_PEM_MCFG(2, 8, 0x894057000000UL),
+ THUNDER_PEM_MCFG(2, 9, 0x89808f000000UL),
+ THUNDER_PEM_MCFG(2, 14, 0x98001f000000UL),
+ THUNDER_PEM_MCFG(2, 15, 0x984057000000UL),
+ THUNDER_PEM_MCFG(2, 16, 0x98808f000000UL),
+ THUNDER_PEM_MCFG(2, 17, 0x99001f000000UL),
+ THUNDER_PEM_MCFG(2, 18, 0x994057000000UL),
+ THUNDER_PEM_MCFG(2, 19, 0x99808f000000UL),
#endif
#ifdef CONFIG_PCI_HOST_THUNDER_ECAM
+#define THUNDER_ECAM_MCFG(rev, seg) \
+ { "CAVIUM", "THUNDERX", rev, seg, MCFG_BUS_ANY, &pci_thunder_ecam_ops }
+
/* SoC pass1.x */
- { "CAVIUM", "THUNDERX", 2, 0, MCFG_BUS_ANY, &pci_thunder_ecam_ops,
- MCFG_RES_EMPTY},
- { "CAVIUM", "THUNDERX", 2, 1, MCFG_BUS_ANY, &pci_thunder_ecam_ops,
- MCFG_RES_EMPTY},
- { "CAVIUM", "THUNDERX", 2, 2, MCFG_BUS_ANY, &pci_thunder_ecam_ops,
- MCFG_RES_EMPTY},
- { "CAVIUM", "THUNDERX", 2, 3, MCFG_BUS_ANY, &pci_thunder_ecam_ops,
- MCFG_RES_EMPTY},
- { "CAVIUM", "THUNDERX", 2, 10, MCFG_BUS_ANY, &pci_thunder_ecam_ops,
- MCFG_RES_EMPTY},
- { "CAVIUM", "THUNDERX", 2, 11, MCFG_BUS_ANY, &pci_thunder_ecam_ops,
- MCFG_RES_EMPTY},
- { "CAVIUM", "THUNDERX", 2, 12, MCFG_BUS_ANY, &pci_thunder_ecam_ops,
- MCFG_RES_EMPTY},
- { "CAVIUM", "THUNDERX", 2, 13, MCFG_BUS_ANY, &pci_thunder_ecam_ops,
- MCFG_RES_EMPTY},
+ THUNDER_ECAM_MCFG(2, 0),
+ THUNDER_ECAM_MCFG(2, 1),
+ THUNDER_ECAM_MCFG(2, 2),
+ THUNDER_ECAM_MCFG(2, 3),
+ THUNDER_ECAM_MCFG(2, 10),
+ THUNDER_ECAM_MCFG(2, 11),
+ THUNDER_ECAM_MCFG(2, 12),
+ THUNDER_ECAM_MCFG(2, 13),
#endif
};
@@ -141,12 +114,12 @@ static void pci_mcfg_match_quirks(struct acpi_pci_root *root,
* table ID, and OEM revision from MCFG table standard header.
*/
for (i = 0, f = mcfg_quirks; i < ARRAY_SIZE(mcfg_quirks); i++, f++) {
- if (f->seg == root->segment &&
- resource_contains(&f->bus_range, &root->secondary) &&
- !memcmp(f->oem_id, mcfg_oem_id, ACPI_OEM_ID_SIZE) &&
+ if (!memcmp(f->oem_id, mcfg_oem_id, ACPI_OEM_ID_SIZE) &&
!memcmp(f->oem_table_id, mcfg_oem_table_id,
ACPI_OEM_TABLE_ID_SIZE) &&
- f->oem_revision == mcfg_oem_revision) {
+ f->oem_revision == mcfg_oem_revision &&
+ f->seg == root->segment &&
+ resource_contains(&f->bus_range, &root->secondary)) {
if (f->cfgres.start)
*cfgres = f->cfgres;
if (f->ops)
@@ -195,10 +168,10 @@ skip_lookup:
}
/*
- * Let to override default ECAM ops and CFG resource range.
- * Also, this might even retrieve CFG resource range in case MCFG
- * does not have it. Invalid CFG start address means MCFG firmware bug
- * or we need another quirk in array.
+ * Allow quirks to override default ECAM ops and CFG resource
+ * range. This may even fabricate a CFG resource range in case
+ * MCFG does not have it. Invalid CFG start address means MCFG
+ * firmware bug or we need another quirk in array.
*/
pci_mcfg_match_quirks(root, &res, &ops);
if (!res.start)
@@ -239,7 +212,7 @@ static __init int pci_mcfg_parse(struct acpi_table_header *header)
/* Save MCFG IDs and revision for quirks matching */
memcpy(mcfg_oem_id, header->oem_id, ACPI_OEM_ID_SIZE);
memcpy(mcfg_oem_table_id, header->oem_table_id, ACPI_OEM_TABLE_ID_SIZE);
- mcfg_oem_revision = header->revision;
+ mcfg_oem_revision = header->oem_revision;
pr_info("MCFG table detected, %d entries\n", n);
return 0;
[toc] | [prev] | [next] | [standalone]
| From | cov@codeaurora.org |
|---|---|
| Date | 2016-09-21 03:20 +0200 |
| Message-ID | <sjEmC-2hJ-11@gated-at.bofh.it> |
| In reply to | #1487568 |
Hi Bjorn, Thomasz,
On 2016-09-20 15:26, Bjorn Helgaas wrote:
> On Fri, Sep 09, 2016 at 09:24:02PM +0200, Tomasz Nowicki wrote:
>> Quirk handling relies on an idea of simple static array which contains
>> quirk enties. Each entry consists of identification information (IDs
>> from
>> standard header of MCFG table) along with custom pci_ecam_ops
>> structure and
>> configuration space resource structure. This way it is possible find
>> corresponding quirk entries and override pci_ecam_ops and PCI
>> configuration
>> space regions.
>>
>> As an example, the last 3 patches present quirk handling mechanism
>> usage for
>> ThunderX.
>>
>> v5 -> v6
>> - rebase against v4.8-rc5
>> - drop patch 1 form previous series
>> - keep pci_acpi_setup_ecam_mapping() in ARM64 arch directory
>> - move quirk code to pci_mcfg.c
>> - restrict quirk to override pci_ecam_ops and CFG resource structure
>> only, no init call any more
>> - split ThunderX quirks into the smaller chunks
>> - add ThunderX pass1.x silicon revision support
>>
>> v4 -> v5
>> - rebase against v4.8-rc1
>> - rework to exact MCFG OEM ID, TABLE ID, rev match
>> - use memcmp instead of strncmp
>> - no substring match
>> - fix typos and dmesg message
>>
>> Tomasz Nowicki (5):
>> PCI/ACPI: Extend pci_mcfg_lookup() responsibilities
>> PCI/ACPI: Check platform specific ECAM quirks
>> PCI: thunder-pem: Allow to probe PEM-specific register range for
>> ACPI
>> case
>> PCI: thunder: Enable ACPI PCI controller for ThunderX pass2.x
>> silicon
>> version
>> PCI: thunder: Enable ACPI PCI controller for ThunderX pass1.x
>> silicon
>> version
>>
>> arch/arm64/kernel/pci.c | 17 ++--
>> drivers/acpi/pci_mcfg.c | 168
>> +++++++++++++++++++++++++++++++++++-
>> drivers/pci/host/pci-thunder-ecam.c | 2 +-
>> drivers/pci/host/pci-thunder-pem.c | 63 +++++++++++---
>> include/linux/pci-acpi.h | 4 +-
>> include/linux/pci-ecam.h | 7 ++
>> 6 files changed, 230 insertions(+), 31 deletions(-)
>
> I'm not quite ready to merge these because we haven't resolved the
> question of how to expose the resources used by the memory-mapped
> config space. I'm fine with the first two patches (I did make a
> couple trivial changes, see below), but there's no point in merging
> them until we merge a user for them.
>
> I pushed the series to pci/ecam-v6 for build testing and discussion.
> The diff (the changes I made locally) from v6 as posted by Tomasz is
> below.
Rebasing the following simple quirks framework user onto this branch,
I have some questions.
https://source.codeaurora.org/quic/server/kernel/commit/?h=cov/4.8-rc2-testing&id=83b766cafef11c107b10177d0626db311f382299
> diff --git a/drivers/acpi/pci_mcfg.c b/drivers/acpi/pci_mcfg.c
> index eb14f74..bb3b8ad 100644
> --- a/drivers/acpi/pci_mcfg.c
> +++ b/drivers/acpi/pci_mcfg.c
> @@ -42,86 +42,59 @@ struct mcfg_fixup {
> struct resource cfgres;
> };
>
> -#define MCFG_DOM_ANY (-1)
Did you delete this because there were no current users, because you'd
prefer users just use "-1", or for some other reason?
> #define MCFG_BUS_RANGE(start, end) DEFINE_RES_NAMED((start), \
> ((end) - (start) + 1), \
> NULL, IORESOURCE_BUS)
> -#define MCFG_BUS_ANY MCFG_BUS_RANGE(0x0, 0xff)
> -#define MCFG_RES_EMPTY DEFINE_RES_NAMED(0, 0, NULL, 0)
> +#define MCFG_BUS_ANY MCFG_BUS_RANGE(0x0, 0xff)
>
> static struct mcfg_fixup mcfg_quirks[] = {
> -/* { OEM_ID, OEM_TABLE_ID, REV, DOMAIN, BUS_RANGE, cfgres, ops }, */
> +/* { OEM_ID, OEM_TABLE_ID, REV, SEGMENT, BUS_RANGE, cfgres, ops }, */
This comment appears to have the order of cfgres and ops reversed.
Am I correct in reading that if a user of the framework does not wish to
override cfgres they must place a struct resource with .start = 0 at the
end of their mcfg_quirks entry? If so, I guess I have the same questions
about removing MCFG_RES_EMPTY as I do about removing MCFG_DOM_ANY.
Thanks,
Cov
[toc] | [prev] | [next] | [standalone]
| From | Bjorn Helgaas <helgaas@kernel.org> |
|---|---|
| Date | 2016-09-21 15:20 +0200 |
| Message-ID | <sjPBn-10O-3@gated-at.bofh.it> |
| In reply to | #1487751 |
On Tue, Sep 20, 2016 at 09:15:14PM -0400, cov@codeaurora.org wrote:
> Hi Bjorn, Thomasz,
>
> On 2016-09-20 15:26, Bjorn Helgaas wrote:
> >On Fri, Sep 09, 2016 at 09:24:02PM +0200, Tomasz Nowicki wrote:
> >>Quirk handling relies on an idea of simple static array which contains
> >>quirk enties. Each entry consists of identification information
> >>(IDs from
> >>standard header of MCFG table) along with custom pci_ecam_ops
> >>structure and
> >>configuration space resource structure. This way it is possible find
> >>corresponding quirk entries and override pci_ecam_ops and PCI
> >>configuration
> >>space regions.
> >>
> >>As an example, the last 3 patches present quirk handling
> >>mechanism usage for
> >>ThunderX.
> >>
> >>v5 -> v6
> >>- rebase against v4.8-rc5
> >>- drop patch 1 form previous series
> >>- keep pci_acpi_setup_ecam_mapping() in ARM64 arch directory
> >>- move quirk code to pci_mcfg.c
> >>- restrict quirk to override pci_ecam_ops and CFG resource structure
> >> only, no init call any more
> >>- split ThunderX quirks into the smaller chunks
> >>- add ThunderX pass1.x silicon revision support
> >>
> >>v4 -> v5
> >>- rebase against v4.8-rc1
> >>- rework to exact MCFG OEM ID, TABLE ID, rev match
> >> - use memcmp instead of strncmp
> >> - no substring match
> >>- fix typos and dmesg message
> >>
> >>Tomasz Nowicki (5):
> >> PCI/ACPI: Extend pci_mcfg_lookup() responsibilities
> >> PCI/ACPI: Check platform specific ECAM quirks
> >> PCI: thunder-pem: Allow to probe PEM-specific register range
> >>for ACPI
> >> case
> >> PCI: thunder: Enable ACPI PCI controller for ThunderX pass2.x
> >>silicon
> >> version
> >> PCI: thunder: Enable ACPI PCI controller for ThunderX pass1.x
> >>silicon
> >> version
> >>
> >> arch/arm64/kernel/pci.c | 17 ++--
> >> drivers/acpi/pci_mcfg.c | 168
> >>+++++++++++++++++++++++++++++++++++-
> >> drivers/pci/host/pci-thunder-ecam.c | 2 +-
> >> drivers/pci/host/pci-thunder-pem.c | 63 +++++++++++---
> >> include/linux/pci-acpi.h | 4 +-
> >> include/linux/pci-ecam.h | 7 ++
> >> 6 files changed, 230 insertions(+), 31 deletions(-)
> >
> >I'm not quite ready to merge these because we haven't resolved the
> >question of how to expose the resources used by the memory-mapped
> >config space. I'm fine with the first two patches (I did make a
> >couple trivial changes, see below), but there's no point in merging
> >them until we merge a user for them.
> >
> >I pushed the series to pci/ecam-v6 for build testing and discussion.
> >The diff (the changes I made locally) from v6 as posted by Tomasz is
> >below.
>
> Rebasing the following simple quirks framework user onto this branch,
> I have some questions.
>
> https://source.codeaurora.org/quic/server/kernel/commit/?h=cov/4.8-rc2-testing&id=83b766cafef11c107b10177d0626db311f382299
>
> >diff --git a/drivers/acpi/pci_mcfg.c b/drivers/acpi/pci_mcfg.c
> >index eb14f74..bb3b8ad 100644
> >--- a/drivers/acpi/pci_mcfg.c
> >+++ b/drivers/acpi/pci_mcfg.c
> >@@ -42,86 +42,59 @@ struct mcfg_fixup {
> > struct resource cfgres;
> > };
> >
> >-#define MCFG_DOM_ANY (-1)
>
> Did you delete this because there were no current users, because you'd
> prefer users just use "-1", or for some other reason?
I removed it because there were no users of it and, more importantly,
the code doesn't implement support for it.
> > #define MCFG_BUS_RANGE(start, end) DEFINE_RES_NAMED((start), \
> > ((end) - (start) + 1), \
> > NULL, IORESOURCE_BUS)
> >-#define MCFG_BUS_ANY MCFG_BUS_RANGE(0x0, 0xff)
> >-#define MCFG_RES_EMPTY DEFINE_RES_NAMED(0, 0, NULL, 0)
> >+#define MCFG_BUS_ANY MCFG_BUS_RANGE(0x0, 0xff)
> >
> > static struct mcfg_fixup mcfg_quirks[] = {
> >-/* { OEM_ID, OEM_TABLE_ID, REV, DOMAIN, BUS_RANGE, cfgres, ops }, */
> >+/* { OEM_ID, OEM_TABLE_ID, REV, SEGMENT, BUS_RANGE, cfgres, ops }, */
>
> This comment appears to have the order of cfgres and ops reversed.
Fixed, thanks!
> Am I correct in reading that if a user of the framework does not wish to
> override cfgres they must place a struct resource with .start = 0 at the
> end of their mcfg_quirks entry? If so, I guess I have the same questions
> about removing MCFG_RES_EMPTY as I do about removing MCFG_DOM_ANY.
You're right that we only override cfgres if the quirk supplies a
struct resource with non-zero .start. I removed MCFG_RES_EMPTY
because mcfg_quirks[] is a static array and is initialized with all
members being zero anyway. If a quirk doesn't need to override
cfgres, I think it's more readable if the quirk just doesn't mention
the resource at all.
Bjorn
[toc] | [prev] | [next] | [standalone]
| From | Sinan Kaya <okaya@codeaurora.org> |
|---|---|
| Date | 2016-09-21 16:10 +0200 |
| Message-ID | <sjQnM-1xc-19@gated-at.bofh.it> |
| In reply to | #1488116 |
On 9/21/2016 9:11 AM, Bjorn Helgaas wrote: > On Tue, Sep 20, 2016 at 09:15:14PM -0400, cov@codeaurora.org wrote: >> Hi Bjorn, Thomasz, >> >> >> Did you delete this because there were no current users, because you'd >> prefer users just use "-1", or for some other reason? > > I removed it because there were no users of it and, more importantly, > the code doesn't implement support for it. Is it possible to queue up Cov's patch as part of this effort once he rebases and sends an updated version? Cov will have to implement something else now. -- Sinan Kaya Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc. Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.
[toc] | [prev] | [next] | [standalone]
| From | Sinan Kaya <okaya@codeaurora.org> |
|---|---|
| Date | 2016-09-21 19:40 +0200 |
| Message-ID | <sjTEZ-3sy-1@gated-at.bofh.it> |
| In reply to | #1488145 |
On 9/21/2016 1:31 PM, Bjorn Helgaas wrote: > On Wed, Sep 21, 2016 at 10:07:36AM -0400, Sinan Kaya wrote: >> On 9/21/2016 9:11 AM, Bjorn Helgaas wrote: >>> On Tue, Sep 20, 2016 at 09:15:14PM -0400, cov@codeaurora.org wrote: >>>> Hi Bjorn, Thomasz, >>>> >> >>>> >>>> Did you delete this because there were no current users, because you'd >>>> prefer users just use "-1", or for some other reason? >>> >>> I removed it because there were no users of it and, more importantly, >>> the code doesn't implement support for it. >> >> Is it possible to queue up Cov's patch as part of this effort once he >> rebases and sends an updated version? Cov will have to implement something >> else now. > > I haven't see Cov's patch (patchwork doesn't follow URLs to git trees, > and I normally don't either). If they show up on the mailing list, > I'll take a look, of course. > > Bjorn > Thanks, I talked to Cov today. He's getting ready to post the rebased patch once he completes testing. -- Sinan Kaya Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc. Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.
[toc] | [prev] | [next] | [standalone]
| From | Bjorn Helgaas <helgaas@kernel.org> |
|---|---|
| Date | 2016-09-21 19:40 +0200 |
| Message-ID | <sjTEZ-3sy-3@gated-at.bofh.it> |
| In reply to | #1488145 |
On Wed, Sep 21, 2016 at 10:07:36AM -0400, Sinan Kaya wrote: > On 9/21/2016 9:11 AM, Bjorn Helgaas wrote: > > On Tue, Sep 20, 2016 at 09:15:14PM -0400, cov@codeaurora.org wrote: > >> Hi Bjorn, Thomasz, > >> > > >> > >> Did you delete this because there were no current users, because you'd > >> prefer users just use "-1", or for some other reason? > > > > I removed it because there were no users of it and, more importantly, > > the code doesn't implement support for it. > > Is it possible to queue up Cov's patch as part of this effort once he > rebases and sends an updated version? Cov will have to implement something > else now. I haven't see Cov's patch (patchwork doesn't follow URLs to git trees, and I normally don't either). If they show up on the mailing list, I'll take a look, of course. Bjorn
[toc] | [prev] | [next] | [standalone]
| From | Christopher Covington <cov@codeaurora.org> |
|---|---|
| Date | 2016-09-22 00:40 +0200 |
| Subject | [PATCHv2] PCI: QDF2432 32 bit config space accessors |
| Message-ID | <sjYlk-6tw-5@gated-at.bofh.it> |
| In reply to | #1488302 |
The Qualcomm Technologies QDF2432 SoC does not support accesses smaller
than 32 bits to the PCI configuration space. Register the appropriate
quirk.
Signed-off-by: Christopher Covington <cov@codeaurora.org>
---
drivers/acpi/pci_mcfg.c | 8 ++++++++
drivers/pci/ecam.c | 10 ++++++++++
include/linux/pci-ecam.h | 1 +
3 files changed, 19 insertions(+)
diff --git a/drivers/acpi/pci_mcfg.c b/drivers/acpi/pci_mcfg.c
index 245b79f..212334f 100644
--- a/drivers/acpi/pci_mcfg.c
+++ b/drivers/acpi/pci_mcfg.c
@@ -96,6 +96,14 @@ static struct mcfg_fixup mcfg_quirks[] = {
THUNDER_ECAM_MCFG(2, 12),
THUNDER_ECAM_MCFG(2, 13),
#endif
+ { "QCOM ", "QDF2432 ", 1, 0, MCFG_BUS_ANY, &pci_32b_ops },
+ { "QCOM ", "QDF2432 ", 1, 1, MCFG_BUS_ANY, &pci_32b_ops },
+ { "QCOM ", "QDF2432 ", 1, 2, MCFG_BUS_ANY, &pci_32b_ops },
+ { "QCOM ", "QDF2432 ", 1, 3, MCFG_BUS_ANY, &pci_32b_ops },
+ { "QCOM ", "QDF2432 ", 1, 4, MCFG_BUS_ANY, &pci_32b_ops },
+ { "QCOM ", "QDF2432 ", 1, 5, MCFG_BUS_ANY, &pci_32b_ops },
+ { "QCOM ", "QDF2432 ", 1, 6, MCFG_BUS_ANY, &pci_32b_ops },
+ { "QCOM ", "QDF2432 ", 1, 7, MCFG_BUS_ANY, &pci_32b_ops },
};
static char mcfg_oem_id[ACPI_OEM_ID_SIZE];
diff --git a/drivers/pci/ecam.c b/drivers/pci/ecam.c
index 43ed08d..c3b3063 100644
--- a/drivers/pci/ecam.c
+++ b/drivers/pci/ecam.c
@@ -162,3 +162,13 @@ struct pci_ecam_ops pci_generic_ecam_ops = {
.write = pci_generic_config_write,
}
};
+
+/* ops for 32 bit config space access quirk */
+struct pci_ecam_ops pci_32b_ops = {
+ .bus_shift = 20,
+ .pci_ops = {
+ .map_bus = pci_ecam_map_bus,
+ .read = pci_generic_config_read32,
+ .write = pci_generic_config_write32,
+ }
+};
diff --git a/include/linux/pci-ecam.h b/include/linux/pci-ecam.h
index 35f0e81..a6cffb8 100644
--- a/include/linux/pci-ecam.h
+++ b/include/linux/pci-ecam.h
@@ -65,6 +65,7 @@ extern struct pci_ecam_ops pci_thunder_pem_ops;
#ifdef CONFIG_PCI_HOST_THUNDER_ECAM
extern struct pci_ecam_ops pci_thunder_ecam_ops;
#endif
+extern struct pci_ecam_ops pci_32b_ops;
#ifdef CONFIG_PCI_HOST_GENERIC
/* for DT-based PCI controllers that support ECAM */
--
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm
Technologies, Inc. Qualcomm Technologies, Inc. is a member of the Code Aurora
Forum, a Linux Foundation Collaborative Project.
[toc] | [prev] | [next] | [standalone]
| From | Christopher Covington <cov@codeaurora.org> |
|---|---|
| Date | 2016-09-22 00:50 +0200 |
| Message-ID | <sjYuZ-6wT-1@gated-at.bofh.it> |
| In reply to | #1488116 |
Hi Bjorn,
On 09/21/2016 09:11 AM, Bjorn Helgaas wrote:
> On Tue, Sep 20, 2016 at 09:15:14PM -0400, cov@codeaurora.org wrote:
>>> diff --git a/drivers/acpi/pci_mcfg.c b/drivers/acpi/pci_mcfg.c
>>> index eb14f74..bb3b8ad 100644
>>> --- a/drivers/acpi/pci_mcfg.c
>>> +++ b/drivers/acpi/pci_mcfg.c
>>> @@ -42,86 +42,59 @@ struct mcfg_fixup {
>>> struct resource cfgres;
>>> };
>>>
>>> -#define MCFG_DOM_ANY (-1)
>>
>> Did you delete this because there were no current users, because you'd
>> prefer users just use "-1", or for some other reason?
>
> I removed it because there were no users of it and, more importantly,
> the code doesn't implement support for it.
It looks like a stale "First match against PCI topology <domain:bus>..."
comment remains.
Thanks,
Cov
--
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm
Technologies, Inc. Qualcomm Technologies, Inc. is a member of the Code
Aurora Forum, a Linux Foundation Collaborative Project.
[toc] | [prev] | [next] | [standalone]
Page 2 of 3 — ← Prev page 1 [2] 3 Next page →
Back to top | Article view | linux.kernel
csiph-web