From mboxrd@z Thu Jan 1 00:00:00 1970 From: Lorenzo Pieralisi Subject: Re: Defining polarity and trigger mode for static interrupts in _PRT Date: Fri, 26 Aug 2016 18:06:15 +0100 Message-ID: <20160826170615.GC23885@red-moon> References: <20160824142723.GA25843@red-moon> <20160824193000.GE23914@localhost> <20160824213005.1a9300ef@arm.com> <20160825121825.322d8450@arm.com> <20160825195917.0e75a8db@arm.com> <20160826090813.GA1038@red-moon> <20160826130832.2fde2327@arm.com> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Return-path: Received: from foss.arm.com ([217.140.101.70]:42130 "EHLO foss.arm.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753842AbcHZRGL (ORCPT ); Fri, 26 Aug 2016 13:06:11 -0400 Content-Disposition: inline In-Reply-To: Sender: linux-acpi-owner@vger.kernel.org List-Id: linux-acpi@vger.kernel.org To: Sinan Kaya Cc: Marc Zyngier , Duc Dang , Bjorn Helgaas , Rafael Wysocki , linux-pci@vger.kernel.org, linux-acpi@vger.kernel.org, patches , Bjorn Helgaas , Punit Agrawal On Fri, Aug 26, 2016 at 10:07:31AM -0400, Sinan Kaya wrote: > On 8/26/2016 8:08 AM, Marc Zyngier wrote: > > On Fri, 26 Aug 2016 10:08:13 +0100 > > Lorenzo Pieralisi wrote: > > > >> [ +Sinan ] > >> > >> On Thu, Aug 25, 2016 at 07:59:17PM +0100, Marc Zyngier wrote: > >> > >> [...] > >> > >>>> Thanks, Marc! It works, I tested on current X-Gene platforms that uses > >>>> GICv2 and GICv2m. > >>>> > >>>> Will you commit this change? It will be a huge help as going with > >>>> interrupt link will require firmware change. > >>> > >>> Not for the time being. We now have an understanding of *why* things do > >>> not work, but Lorenzo seems to have a good grasp on what we can do to > >>> address it correctly, and we should explore this first. If (and only if) > >>> there is a consensus that firmware already does all it should, then > >>> I'll turn this hack into a proper series. > >> > >> For the records, it is a discussion that already took place: > >> > >> https://lkml.org/lkml/2016/3/14/923 > >> > >> As I have said there are already ARM64 systems with ACPI tables > >> out there using PCI interrupt links; I doubt that Qualcomm systems > >> allow to reconfigure the GIC interrupt pin allocated to legacy PCI > >> IRQs through interrupt links _SRS (hey it is *empty* :)), > >> therefore: > >> > >> a) Some (the above is just an example from the mailing lists I am not > >> picking on anyone it is just for the sake of this discussion, I have > >> not dumped all ARM partners _PRT from their ACPI tables to check) > >> ACPI tables must be rewritten because they are not FW compliant > >> > >> OR > >> > >> b) We allow PCI interrupt links to be used for static interrupt > >> configurations > >> > >> OR > >> > >> c) We ignore the polarity flag (only for PCI legacy IRQs ? I wonder > >> how GIC code can detect from which part of the kernel the interrupt > >> request is coming, unless we implement an ACPI-PCI-legacy-IRQ-special > >> gem) > >> > >> Comments ? > > > > I'm not overly keen on (c), as it is pretty hard to find out where the > > request is coming from (and the hack I previously posted opens the door > > to all kind of undetected misconfiguration). We *could* use a stacked > > irqchip to represent the inverter, but it feels like using a car sized > > hammer to squash a tiny fly. > > > > (b) seems like the right thing to do, but I cannot comment on whether > > or not this is compliant with the specification. > > > > Thanks, > > > > M. > > > > Let me throw option d here. > > I know Bjorn wants to keep ACTIVE_LOW in the code for common code but > can't we override this in an arch specific way (arm64's pci.c) while > creating the root bridge? On what basis ? You were not copied in from the beginning, but that is not different from Duc's initial proposal, which Marc discarded because it should not be done at arch level, it depends on the interrupt controller. Possibly a hook to be called from GIC code to override the default ACPI PCI IRQ polarity, I think that's _horrible_ but if we want to successfully boot APM's platforms that are $SUBJECT of this thread something has to be done (and it is not patching FW because we can't). > If the ARCH override doesn't exist, ACTIVE LOW still remains the > default. There could be another arch that could have the same problem > in the future. See above. > This way, we don't need to touch irqchip (GIC) driver or introduce a > new API and/or introduce bugs for the rest of the non-PCI code. > > From what I see in the ACPI spec, both _PRT approaches are correct and > they need to be supported by Linux. They are; ..but the spec says that your ACPI tables are buggy, because you are using a PCI interrupt link for an interrupt that is not configurable (frankly I still do not understand why as I explained). And then there is FW that has already shipped and we can't patch and it is not using PCI interrupt links so we have to quirk it in the kernel somehow (ie I am not sure APM platforms are the only ones containing _PRT with static PCI legacy IRQ entries, unfortunately). Lorenzo