| Message ID | 1456401208-10136-1-git-send-email-thunder.leizhen@huawei.com |
|---|---|
| State | New |
| Headers | show |
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. 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 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? > Signed-off-by: Zhen Lei <thunder.leizhen@huawei.com> > --- > 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 : <see aforementioned specification> > > +- 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. 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 > -- 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
On Fri, Feb 26, 2016 at 03:19:55PM +0800, Leizhen (ThunderTown) wrote: > > 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 <PCI Local Bus Specification Revision 3.0>: > 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? Per the above, that does appear to be the case. > 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. I assume that for devices with 0 in this register we do not produce a warning. So where do we check the interrupt pin register, and when does this happen relative to of_irq_parse_pci such that we do not produce that warning? I als assume that all instances of these particular devices broken in this regard? If so, I think we need to identify them by Device ID and Vendor ID, and treat them as if the interrupt pin register read as zero, in the place we normally check the interrupt pin register. Note that this is completely independent of the RID/BDF, so the interrupt-*-mask approach is insufficient. > 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. I'm not entirely sure, but I thought we handled the PCI I/O space as an MMIO region on ARM64. Do you have many devices/functions attached? It may be that our VA carveout of 16M is too small. This is probably worth a separate thread. > >> 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. This is very fragile, as it depends on the RIDs/addresses assigned by the host controller. If devices are plugged into different slots then that could change, you get the warning, and other devices may be prevented from using wired interrupts. As I mentioned above, I think we need to identify the buggy devices by ID, rather than by topology. Thanks, Mark. -- 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
diff --git a/drivers/of/of_pci_irq.c b/drivers/of/of_pci_irq.c index 2306313..a6d51e0 100644 --- a/drivers/of/of_pci_irq.c +++ b/drivers/of/of_pci_irq.c @@ -4,6 +4,38 @@ #include <linux/export.h> /** + * of_irq_skip_pci - Skip the interrupt pins for a PCI device + * @pdev: the device whose interrupt pins may be skipped + * @dn: device node of pdev or pdev's ancestral bus + * + * Interrupt Pin register is read-only and optional. Some pci devices may use + * msi/msix but leave the value of Interrupt Pin non-zero. This function give + * an opportunity to suppress the warning about of_irq_parse_pci() failed. + */ +static int of_irq_skip_pci(const struct pci_dev *pdev, + const struct device_node *dn) +{ + const __be32 *skip_mask, *end; + u32 addr, mask; + int len; + + skip_mask = of_get_property(dn, "interrupt-skip-mask", &len); + if (!skip_mask) + return 0; + + end = skip_mask + (len / sizeof(__be32)); + addr = (pdev->bus->number << 16) | (pdev->devfn << 8); + + while (skip_mask < end) { + mask = be32_to_cpu(*(skip_mask++)); + if ((addr & ~mask) == 0) + return 1; + } + + return 0; +} + +/** * of_irq_parse_pci - Resolve the interrupt for a PCI device * @pdev: the device whose interrupt is to be resolved * @out_irq: structure of_irq filled by this function @@ -30,6 +62,9 @@ int of_irq_parse_pci(const struct pci_dev *pdev, struct of_phandle_args *out_irq rc = of_irq_parse_one(dn, 0, out_irq); if (!rc) return rc; + + if (of_irq_skip_pci(pdev, dn)) + return -ENODEV; } /* Ok, we don't, time to have fun. Let's start by building up an @@ -89,9 +124,11 @@ int of_irq_parse_pci(const struct pci_dev *pdev, struct of_phandle_args *out_irq laddr[0] = cpu_to_be32((pdev->bus->number << 16) | (pdev->devfn << 8)); laddr[1] = laddr[2] = cpu_to_be32(0); rc = of_irq_parse_raw(laddr, out_irq); - if (rc) - goto err; - return 0; + if (!rc) + return 0; + + if (of_irq_skip_pci(pdev, ppnode)) + return -ENODEV; err: dev_err(&pdev->dev, "of_irq_parse_pci() failed with rc=%d\n", rc); return rc;
Interrupt Pin register is read-only and optional. Some pci devices may use msi/msix but leave the value of Interrupt Pin non-zero. 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. Signed-off-by: Zhen Lei <thunder.leizhen@huawei.com> --- drivers/of/of_pci_irq.c | 43 ++++++++++++++++++++++++++++++++++++++++--- 1 file changed, 40 insertions(+), 3 deletions(-) -- 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