Hi Lorenzo

-----Original Message-----
From: linux-kernel-owner at vger.kernel.org [mailto:linux-kernel-
owner at 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 at vger.kernel.org; linux-arm-kernel at lists.infradead.org; Linaro ACPI
Mailman List; Jon Masters; Andrea Gallo; Jeremy Linton; liudongdong
(C); Jeff Hugo; linux-acpi at vger.kernel.org; linux-
kernel at vger.kernel.org; Rafael J. Wysocki; rui.zhang at 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:

quoted

On Thu, Sep 22, 2016 at 01:31:01PM -0500, Bjorn Helgaas wrote:

quoted

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

quoted

quoted

quoted

quoted

quoted

Marinas; Rafael Wysocki; Arnd Bergmann; Hanjun Guo; Sinan

Kaya;

quoted

quoted

quoted

quoted

quoted

Jayachandran C; Christopher Covington; Duc Dang; Robert

Richter; Marcin

quoted

quoted

quoted

quoted

quoted

Wojtas; Liviu Dudau; Wangyijing; Mark Salter; linux-
pci at vger.kernel.org; linux-arm-kernel at lists.infradead.org;

Linaro ACPI

quoted

quoted

quoted

quoted

quoted

Mailman List; Jon Masters; Andrea Gallo; Jeremy Linton;

liudongdong

quoted

quoted

quoted

quoted

quoted

(C); Gabriele Paoloni; Jeff Hugo; linux-acpi at vger.kernel.org;

linux-

quoted

quoted

quoted

quoted

quoted

kernel at vger.kernel.org; Rafael J. Wysocki
Subject: Re: [PATCH V6 3/5] PCI: thunder-pem: Allow to probe

PEM-

quoted

quoted

quoted

quoted

quoted

specific register range for ACPI case

On Wed, Sep 21, 2016 at 01:04:57PM -0500, Bjorn Helgaas

wrote:

quoted

quoted

quoted

quoted

quoted

quoted

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

wrote:

quoted

quoted

quoted

quoted

quoted

quoted

quoted

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

wrote:

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

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

Biesheuvel wrote:

quoted

quoted

quoted

quoted

quoted

quoted

quoted

[...]

quoted

quoted

None of these platforms can be fixed entirely in

software, and

quoted

quoted

quoted

quoted

quoted

given

quoted

quoted

quoted

quoted

that we will not be adding quirks for new broken

hardware, we

quoted

quoted

quoted

quoted

quoted

should

quoted

quoted

quoted

quoted

ask ourselves whether having two versions of a quirk,

i.e., one

quoted

quoted

quoted

quoted

quoted

for

quoted

quoted

quoted

quoted

broken hardware + currently shipping firmware, and

one for the

quoted

quoted

quoted

quoted

quoted

same

quoted

quoted

quoted

quoted

broken hardware with fixed firmware is really an

improvement

quoted

quoted

quoted

quoted

quoted

over what

quoted

quoted

quoted

quoted

has been proposed here.

We're talking about two completely different types of

quirks:

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

  1) MCFG quirks to use memory-mapped config space that

doesn't

quoted

quoted

quoted

quoted

quoted

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

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

     consumed by a bridge.

The first two patches of this series are a nice

implementation

quoted

quoted

quoted

quoted

quoted

for 1).

quoted

quoted

quoted

The third patch (ThunderX-specific) is one possibility

for 2),

quoted

quoted

quoted

quoted

quoted

but I

quoted

quoted

quoted

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

software like

quoted

quoted

quoted

quoted

quoted

the

quoted

quoted

quoted

ACPI core to discover these resources.

Ok, so basically this means that to implement (2) we need

to assign

quoted

quoted

quoted

quoted

quoted

quoted

quoted

some sort of _HID to these quirky PCI bridges (so that we

know what

quoted

quoted

quoted

quoted

quoted

quoted

quoted

device they represent and we can retrieve their _CRS). I

take from

quoted

quoted

quoted

quoted

quoted

quoted

quoted

this discussion that the goal is to make sure that all

non-config

quoted

quoted

quoted

quoted

quoted

quoted

quoted

resources have to be declared through _CRS device

objects, which is

quoted

quoted

quoted

quoted

quoted

quoted

quoted

fine but that requires a FW update (unless we can

fabricate ACPI

quoted

quoted

quoted

quoted

quoted

quoted

quoted

devices and corresponding _CRS in the kernel whenever we

match a

quoted

quoted

quoted

quoted

quoted

quoted

quoted

given MCFG table signature).

All resources consumed by ACPI devices should be declared

through

quoted

quoted

quoted

quoted

quoted

quoted

_CRS.  If you want to fabricate ACPI devices or _CRS via

kernel

quoted

quoted

quoted

quoted

quoted

quoted

quirks, that's fine with me.  This could be triggered via

MCFG

quoted

quoted

quoted

quoted

quoted

quoted

signature, DMI info, host bridge _HID, etc.

I think the PNP quirk approach + PNP0c02 resource put forward

by Gab

quoted

quoted

quoted

quoted

quoted

is enough.

Great thanks as we take a final decision I will ask Dogndgong

to submit

quoted

quoted

quoted

quoted

another RFC based on this approach

quoted

quoted

quoted

We discussed this already and I think we should make a

decision:

quoted

quoted

quoted

quoted

quoted

quoted

quoted

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

kernel/2016-

quoted

quoted

quoted

quoted

quoted

March/414722.html

quoted

quoted

quoted

quoted

quoted

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

understanding of

quoted

quoted

quoted

quoted

quoted

how

quoted

quoted

quoted

quoted

quoted

non-broken firmware *should* deal with this issue.

Then, if

quoted

quoted

quoted

quoted

quoted

we *do*

quoted

quoted

quoted

quoted

quoted

work around this particular broken firmware in the

kernel, it

quoted

quoted

quoted

quoted

quoted

would be

quoted

quoted

quoted

quoted

quoted

nice to do it in a way that fits in with that

understanding.

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

For example, if a companion ACPI device is the

preferred

quoted

quoted

quoted

quoted

quoted

solution, an

quoted

quoted

quoted

quoted

quoted

ACPI quirk could fabricate a device with the

required

quoted

quoted

quoted

quoted

quoted

resources.  That

quoted

quoted

quoted

quoted

quoted

would address the problem closer to the source and

make it

quoted

quoted

quoted

quoted

quoted

more likely

quoted

quoted

quoted

quoted

quoted

that the rest of the system will work correctly:

/proc/iomem

quoted

quoted

quoted

quoted

quoted

could

quoted

quoted

quoted

quoted

quoted

make sense, things that look at _CRS generically

would work

quoted

quoted

quoted

quoted

quoted

(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

quoted

quoted

quoted

quoted

quoted

provide

quoted

quoted

quoted

quoted

quoted

any guidance for future platforms and makes it

likely that

quoted

quoted

quoted

quoted

quoted

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

quoted

quoted

quoted

quoted

quoted

not

quoted

quoted

quoted

quoted

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

the more we

quoted

quoted

quoted

quoted

quoted

polish

quoted

quoted

quoted

quoted

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

likely it

quoted

quoted

quoted

quoted

quoted

is

quoted

quoted

quoted

quoted

that will end up getting abused by new broken

hardware that we

quoted

quoted

quoted

quoted

quoted

set out

quoted

quoted

quoted

quoted

to reject entirely in the first place.

So of course, if the quirk involves claiming

resources, let's

quoted

quoted

quoted

quoted

quoted

make

quoted

quoted

quoted

quoted

sure that this occurs in the cleanest and most

compliant way

quoted

quoted

quoted

quoted

quoted

possible.

quoted

quoted

quoted

quoted

But any factoring/reuse concerns other than for the

current

quoted

quoted

quoted

quoted

quoted

crop of

quoted

quoted

quoted

quoted

broken hardware should be avoided imo.

If future hardware is completely ECAM-compliant and we

don't need

quoted

quoted

quoted

quoted

quoted

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

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

somewhere.  If that's done with PNP0C02 or similar

devices (as is

quoted

quoted

quoted

quoted

quoted

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

quoted

quoted

quoted

quoted

quoted

quoted

quoted

to MCFG regions reported as PNP0c02 resources through its

_CRS ?

quoted

quoted

quoted

quoted

quoted

quoted

Yes.  PCI Firmware Spec r3.0, Table 4-2, note 2 says

address ranges

quoted

quoted

quoted

quoted

quoted

quoted

reported via MCFG or _CBA should be reserved by _CRS of a

PNP0C02

quoted

quoted

quoted

quoted

quoted

quoted

device.

Ok, that's agreed. It goes without saying that since you are

quoting

quoted

quoted

quoted

quoted

quoted

the PCI spec, if FW fails to report MCFG regions in a PNP0c02

device

quoted

quoted

quoted

quoted

quoted

_CRS I will consider that a FW bug.

quoted

quoted

IIUC PNP0C02 is a reservation mechanism, but it does not

help us

quoted

quoted

quoted

quoted

quoted

quoted

quoted

associate its _CRS to a specific PCI host bridge

instance, right ?

quoted

quoted

quoted

quoted

quoted

quoted

Gab proposed a hierarchy that *would* associate a PNP0C02

device with

quoted

quoted

quoted

quoted

quoted

quoted

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

quoted

quoted

quoted

quoted

quoted

(windows) }

quoted

    Device (RES0) {
      Name (_HID, "HISI0081") // HiSi PCIe RC config base

address

quoted

quoted

quoted

quoted

quoted

quoted

      Name (_CID, "PNP0C02")  // Motherboard reserved

resource

quoted

quoted

quoted

quoted

quoted

quoted

      Name (_CRS, ResourceTemplate () { ... }
    }
  }

That's a possibility.  The PCI Firmware Spec suggests

putting RES0 at

quoted

quoted

quoted

quoted

quoted

quoted

the root (under \_SB), but I don't know why.

Putting it at the root means we couldn't generically

associate it

quoted

quoted

quoted

quoted

quoted

with

quoted

a bridge, although I could imagine something like this:

  Device (RES1) {
    Name (_HID, "HISI0081") // HiSi PCIe RC config base

address

quoted

quoted

quoted

quoted

quoted

quoted

    Name (_CID, "PNP0C02")  // Motherboard reserved

resource

quoted

quoted

quoted

quoted

quoted

quoted

    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

quoted

quoted

quoted

quoted

quoted

(windows) }

quoted

  }

Where you could search PNP0C02 devices for a cookie that

matched the

quoted

quoted

quoted

quoted

quoted

quoted

host bridge.o

Ok, I am fine with both and I think we are converging, but

the way

quoted

quoted

quoted

quoted

quoted

to solve this problem has to be uniform for all ARM partners

(and

quoted

quoted

quoted

quoted

quoted

not only ARM). Two points here:

1) Adding a device/subdevice allows people to add a _CRS

reporting the

quoted

quoted

quoted

quoted

quoted

   non-window bridge resources. Fine. It also allows people

to chuck in

quoted

quoted

quoted

quoted

quoted

   there all sorts of _DSD properties to describe their PCI

host bridge

quoted

quoted

quoted

quoted

quoted

   as it is done with DT properties (those _DSD can contain

eg clocks

quoted

quoted

quoted

quoted

quoted

   etc.), this may be tempting (so that they can reuse the

same DT

quoted

quoted

quoted

quoted

quoted

   driver and do not have to update their firmware) but I

want to be

quoted

quoted

quoted

quoted

quoted

   clear here: that must not happen. So, a subdevice with a

_CRS to

quoted

quoted

quoted

quoted

quoted

   report resources, yes, but it will stop there.
2) It is unclear to me how to formalize the above. People

should not

quoted

quoted

quoted

quoted

quoted

   write FW by reading the PCI mailing list, so these

guidelines have

quoted

quoted

quoted

quoted

quoted

to
   be written, somehow. I do not want to standardize quirks,

I want

quoted

quoted

quoted

quoted

quoted

   to prevent random ACPI table content, which is different.
   Should I report this to the ACPI spec working group ? If

we do

quoted

quoted

quoted

quoted

quoted

   not do that everyone will go solve this problem as they

deem fit.

quoted

quoted

quoted

quoted

quoted

Do we really need to formalize this?

As we discussed in the Linaro call at the moment we have few

vendors

quoted

quoted

quoted

quoted

that need quirks and we want to avoid promoting/accepting

quirks for

quoted

quoted

quoted

quoted

the future.

At the time of the call I think we decided to informally accept

a set

quoted

quoted

quoted

quoted

of quirks for the current platforms and reject any other quirk

coming

quoted

quoted

quoted

quoted

after a certain date/kernel version (this to be decided).

I am not sure if there is a way to document/formalize a

temporary

quoted

quoted

quoted

quoted

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

quoted

quoted

the hardware doesn't support ECAM quite according to the spec.

PNP0C02 usage is a workaround for the failure of the

Consumer/Producer

quoted

quoted

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.

quoted

quoted

Apparently x86 hasn't needed it yet.

If people add _DSD methods for clocks or whatnot, the hardware

won't

quoted

quoted

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