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:

quoted

On Thu, Sep 22, 2016 at 11:10:13AM +0000, Gabriele Paoloni wrote:

quoted

Hi Lorenzo, Bjorn

quoted

-----Original Message-----
From: Lorenzo Pieralisi [mailto:lorenzo.pieralisi at 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 at vger.kernel.org; linux-arm-kernel at lists.infradead.org; Linaro ACPI
Mailman List; Jon Masters; Andrea Gallo; Jeremy Linton; liudongdong
(C); Gabriele Paoloni; Jeff Hugo; linux-acpi at vger.kernel.org; linux-
kernel at 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:

quoted

On Wed, Sep 21, 2016 at 03:05:49PM +0100, Lorenzo Pieralisi wrote:

quoted

On Tue, Sep 20, 2016 at 02:17:44PM -0500, Bjorn Helgaas wrote:

quoted

On Tue, Sep 20, 2016 at 04:09:25PM +0100, Ard Biesheuvel wrote:

[...]

quoted

quoted

None of these platforms can be fixed entirely in software, and

given

quoted

quoted

quoted

quoted

that we will not be adding quirks for new broken hardware, we

should

quoted

quoted

quoted

quoted

ask ourselves whether having two versions of a quirk, i.e., one

for

quoted

quoted

quoted

quoted

broken hardware + currently shipping firmware, and one for the

same

quoted

quoted

quoted

quoted

broken hardware with fixed firmware is really an improvement

over what

quoted

quoted

quoted

quoted

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

quoted

quoted

quoted

     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).

quoted

quoted

quoted

The third patch (ThunderX-specific) is one possibility for 2),

but I

quoted

quoted

quoted

don't like it because there's no way for generic software like

the

quoted

quoted

quoted

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

quoted

quoted

quoted

We discussed this already and I think we should make a decision:

http://lists.infradead.org/pipermail/linux-arm-kernel/2016-

March/414722.html

quoted

quoted

quoted

quoted

quoted

I'd like to step back and come up with some understanding of

how

quoted

quoted

quoted

quoted

quoted

non-broken firmware *should* deal with this issue.  Then, if

we *do*

quoted

quoted

quoted

quoted

quoted

work around this particular broken firmware in the kernel, it

would be

quoted

quoted

quoted

quoted

quoted

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

quoted

quoted

quoted

quoted

quoted

ACPI quirk could fabricate a device with the required

resources.  That

quoted

quoted

quoted

quoted

quoted

would address the problem closer to the source and make it

more likely

quoted

quoted

quoted

quoted

quoted

that the rest of the system will work correctly: /proc/iomem

could

quoted

quoted

quoted

quoted

quoted

make sense, things that look at _CRS generically would work

(e.g,

quoted

quoted

quoted

quoted

quoted

/sys/, an admittedly hypothetical "lsacpi", etc.)

Hard-coding stuff in drivers is a point solution that doesn't

provide

quoted

quoted

quoted

quoted

quoted

any guidance for future platforms and makes it likely that

the hack

quoted

quoted

quoted

quoted

quoted

will get copied into even more drivers.

OK, I see. But the guidance for future platforms should be 'do

not

quoted

quoted

quoted

quoted

rely on quirks', and what I am arguing here is that the more we

polish

quoted

quoted

quoted

quoted

up this code and make it clean and reusable, the more likely it

is

quoted

quoted

quoted

quoted

that will end up getting abused by new broken hardware that we

set out

quoted

quoted

quoted

quoted

to reject entirely in the first place.

So of course, if the quirk involves claiming resources, let's

make

quoted

quoted

quoted

quoted

sure that this occurs in the cleanest and most compliant way

possible.

quoted

quoted

quoted

quoted

But any factoring/reuse concerns other than for the current

crop of

quoted

quoted

quoted

quoted

broken hardware should be avoided imo.

If future hardware is completely ECAM-compliant and we don't need

any

quoted

quoted

quoted

more MCFG quirks, that would be great.

Yes.

quoted

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

quoted

quoted

quoted

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.

quoted

quoted

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) }

quoted

    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

quoted

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) }

quoted

  }

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