From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S938700AbcIWK7S convert rfc822-to-8bit (ORCPT ); Fri, 23 Sep 2016 06:59:18 -0400 Received: from lhrrgout.huawei.com ([194.213.3.17]:3532 "EHLO lhrrgout.huawei.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932390AbcIWK7P (ORCPT ); Fri, 23 Sep 2016 06:59:15 -0400 From: Gabriele Paoloni To: Lorenzo Pieralisi , 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)" , 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 Thread-Topic: [PATCH V6 3/5] PCI: thunder-pem: Allow to probe PEM-specific register range for ACPI case Thread-Index: AQHSCs/bKvgtuUMNE0KZWVrE8/EhzKCBG2cAgADd8ICAAGdogIAAAeSAgAAHAgCAABHqgIAARWEAgAE7L4CAAELQgIABCAWAgAAi/tCAAA3dAIAAYL2AgAA9YQCAAMl2gIAAG5KQ Date: Fri, 23 Sep 2016 10:58:28 +0000 Message-ID: References: <20160920140518.GD13855@localhost> <20160920191744.GA4941@localhost> <20160921140549.GA11968@red-moon> <20160921180457.GB20006@localhost> <20160922094955.GB18637@red-moon> <20160922124446.GA21681@red-moon> <20160922183101.GA14510@localhost> <20160922221042.GA1514@localhost> <20160923101145.GA31120@red-moon> In-Reply-To: <20160923101145.GA31120@red-moon> Accept-Language: en-GB, en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [10.203.181.151] Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 8BIT MIME-Version: 1.0 X-CFilter-Loop: Reflected X-Mirapoint-Virus-RAPID-Raw: score=unknown(0), refid=str=0001.0A020205.57E50AE5.00AC,ss=1,re=0.000,recu=0.000,reip=0.000,cl=1,cld=1,fgs=0, ip=0.0.0.0, so=2013-06-18 04:22:30, dmn=2013-03-21 17:37:32 X-Mirapoint-Loop-Id: d9bbc532842c60c4d33b4c2a4b8f8a09 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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