From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932825AbcBZHUj (ORCPT ); Fri, 26 Feb 2016 02:20:39 -0500 Received: from szxga02-in.huawei.com ([119.145.14.65]:41855 "EHLO szxga02-in.huawei.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753592AbcBZHUh (ORCPT ); Fri, 26 Feb 2016 02:20:37 -0500 Subject: Re: [PATCH 2/2] PCI: generic: add description of property "interrupt-skip-mask" To: Mark Rutland References: <1456401208-10136-1-git-send-email-thunder.leizhen@huawei.com> <1456401208-10136-2-git-send-email-thunder.leizhen@huawei.com> <20160225122035.GB10593@leverpostej> CC: Bjorn Helgaas , linux-pci , Rob Herring , Frank Rowand , Grant Likely , Will Deacon , Pawel Moll , Ian Campbell , Kumar Gala , linux-arm-kernel , devicetree , linux-kernel , Zefan Li , Xinwei Hu , Tianhong Ding , Hanjun Guo , Yijing Wang From: "Leizhen (ThunderTown)" Message-ID: <56CFFC9B.20703@huawei.com> Date: Fri, 26 Feb 2016 15:19:55 +0800 User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:38.0) Gecko/20100101 Thunderbird/38.5.1 MIME-Version: 1.0 In-Reply-To: <20160225122035.GB10593@leverpostej> Content-Type: text/plain; charset="windows-1252" Content-Transfer-Encoding: 7bit X-Originating-IP: [10.177.23.164] X-CFilter-Loop: Reflected X-Mirapoint-Virus-RAPID-Raw: score=unknown(0), refid=str=0001.0A090205.56CFFCB0.0027,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: 45cf7bfe97eda95edeeec78020cf9c1e Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 2016/2/25 20:20, Mark Rutland wrote: > Hi, > > In future, please send the binding document first in a series, per point > 3 of Documentation/devicetree/bindings/submitting-patches.txt. It makes > review easier/faster. Thank you for your reminding. > > On Thu, Feb 25, 2016 at 07:53:28PM +0800, Zhen Lei wrote: >> Interrupt Pin register is read-only and optional. Some pci devices may use >> msi/msix but leave the value of Interrupt Pin non-zero. > > Is that permitted by the spec? Surely 'optional' means it must be zero > if not implemented? In : Devices (or device functions) that do not use an interrupt pin must put a 0 in this register. This register is read-only. So, do you think this is a hardware bug? But these pci-devices are not produced by our company. In function init_service_irqs, it try msix first, then msi, Interrupt PIN is the last attemption. But of_irq_parse_pci() happened before this. In fact, there also a familiar problem exist. As below: pci 0000:42:00.0: BAR 7: no space for [io size 0x1000] pci 0000:42:00.0: BAR 7: failed to assign [io size 0x1000] There no "io space" on arm64, maybe only exist on X86. And the Memory Space Indicator also read-only in BAR register. > >> In this case, the driver will print information as below: pci >> 0000:40:00.0: of_irq_parse_pci() failed with rc=-22 >> >> It's easily lead to misinterpret. > > If this is limited to a subset of devices which we know are broken in > this regard, can we not handle these cases explicitly? Actually, we have another way to block this warning. Use "interrupt-map" to map it to a pesudo IRQ. But I think it will also be misunderstanded. > >> Signed-off-by: Zhen Lei >> --- >> Documentation/devicetree/bindings/pci/host-generic-pci.txt | 2 ++ >> 1 file changed, 2 insertions(+) >> >> diff --git a/Documentation/devicetree/bindings/pci/host-generic-pci.txt b/Documentation/devicetree/bindings/pci/host-generic-pci.txt >> index 3f1d3fc..0f10978 100644 >> --- a/Documentation/devicetree/bindings/pci/host-generic-pci.txt >> +++ b/Documentation/devicetree/bindings/pci/host-generic-pci.txt >> @@ -70,6 +70,8 @@ Practice: Interrupt Mapping' and requires the following properties: >> >> - interrupt-map-mask : >> >> +- interrupt-skip-mask: Explicitly declare which pci devices only use msi/msix >> +but leave the value of Interrupt Pin non-zero. > > Unlike the rest of the interrupt mapping properties, this is not > described in `Open Firmware Recommended Practice: Interrupt Mapping'. > > This needs a far more complete description. > > This also doesn't strike me as th right approach. The interrupt-map-mask > property describe as relationship between the host-controller-provided > interrupt lines and endpoints, while this seems to be a bug completely > contained within an endpoint. In : // PCI_DEVICE(3) INT#(1) CONTROLLER(PHANDLE) CONTROLLER_DATA(3) interrupt-map = < 0x0 0x0 0x0 0x1 &gic 0x0 0x4 0x1 PCI_DEVICE contain 3 cells. But only the first one be used in function of_irq_parse_pci. laddr[0] = cpu_to_be32((pdev->bus->number << 16) | (pdev->devfn << 8)); laddr[1] = laddr[2] = cpu_to_be32(0); And for INT#, I don't think there will some Pins used but others unused on a pci-device. So I can ommit it. So, only laddr[0] mask need to be described. > > Thanks, > Mark. > >> >> Example: >> >> -- >> 2.5.0 >> >> >> -- >> To unsubscribe from this list: send the line "unsubscribe devicetree" in >> the body of a message to majordomo@vger.kernel.org >> More majordomo info at http://vger.kernel.org/majordomo-info.html >> > > . >